[SYCL] Immutable kernel functions redo. #2259

cperkinsintel · 2020-08-04T23:39:35Z

This is the same immutable kernel functions PR that was merged (and reverted) last week. The SYCL conformance tests have been updated, so hopefully this should be mergeable. I've added the __SYCL_NONCONST_FUNCTOR__ as requested.

I have also hooked up the -sycl-std= compiler driver flag so that it will pass DCL_SYCL_LANGUAGE_VERSION=121 if SYCL 1.2.1 is chosen or DCL_SYCL_LANGUAGE_VERSION=2020 if SYCL 2020 is chosen.

In SYCL 2020 kernel functions are now const and passed by reference. In this PR the existing API constant CL_SYCL_LANGUAGE_VERSION now determine another constant ( SYCL_NONCONST_FUNCTOR ) that makes the kernel functions be constant references (or not).

In the Clang Front end, both by-reference and by-value passing of the kernel functions is supported, and a diagnostic emitted if that is mismatched. The sycl-std flag now supports 2020 as a valid option. And the sycl-std flag will pass -DCL_SYCL_LANGUAGE_VERSION= to match its election. Tests have been added to confirm that both kernel passings are supported and that the sycl-std flag is working correctly. Additionally, there are a lot of FE tests that employ mock kernels, these have been updated to the SYCL 2020 convention avoiding the deferred maintenance.

Signed-off-by: Chris Perkins chris.perkins@intel.com

sycl/include/CL/sycl/queue.hpp

cperkinsintel · 2020-08-06T00:36:57Z

There is a warning that is being emitted by ExternalASTSource.h when building the ASTImporterTest.cpp unit test. I wasn't seeing this until I rebased. However, it seems to be in the sycl branch of intel/llvm.

sycl/include/CL/sycl/handler.hpp

Fznamznon

FE changes LGTM.

clang/lib/Driver/ToolChains/Clang.cpp

bader · 2020-08-10T19:31:46Z

clang/test/CodeGenSYCL/kernel-by-reference.cpp

+// SYCL 2020 - kernel functions are passed by reference.
+template <typename name, typename Func>
+#if defined(SYCL2017)
+// expected-warning@+2 {{Passing of kernel functions by reference is a SYCL 2020 extension}}


This probably should be emitted as a note or only in pedantic mode. This API can be back-ported to SYCL-1.2.1 mode (as it's done in our implementation if I understand the patch), but it might be supported by other implementations. IIRC, DPC++ compiler sets -Wsycl-strict if -sycl-std= option is set. Am I right?

I'd like to hear @erichkeane opinion on this.

Suggested change

// expected-warning@+2 {{Passing of kernel functions by reference is a SYCL 2020 extension}}

// expected-warning@+2 {{Passing of kernel functions by reference is a SYCL 2020 extension}}

"extension"? I think this is standard SYCL-2020 API.

clang/test/SemaSYCL/accessor-type-diagnostics.cpp

clang/test/CodeGenSYCL/sycl-std-flag.cpp

clang/test/SemaSYCL/implicit_kernel_type.cpp

sycl/include/CL/sycl/handler.hpp

erichkeane · 2020-08-12T23:15:10Z

clang/lib/Sema/SemaSYCL.cpp

@@ -707,7 +707,39 @@ getKernelInvocationKind(FunctionDecl *KernelCallerFunc) {

 static const CXXRecordDecl *getKernelObjectType(FunctionDecl *Caller) {
  assert(Caller->getNumParams() > 0 && "Insufficient kernel parameters");
-  return Caller->getParamDecl(0)->getType()->getAsCXXRecordDecl();
+
+  QualType KernelParamTy = (*Caller->param_begin())->getType();


I don't know if you've done the re-merge yet, but this line changes a bit to Caller->getParamDecl(0)->getType().

clang/lib/Sema/SemaSYCL.cpp

erichkeane

Just scrolled through the test changes, but the CFE parts look ok to me. Properly rebased as well it appears.

erichkeane · 2020-08-13T00:57:49Z

clang/lib/Sema/SemaSYCL.cpp

+  if (KernelParamTy->isReferenceType()) {
+    // passing by reference, so emit warning if not using SYCL 2020
+    if (LangOpts.SYCLVersion < 2020)
+      Diag(KernelFunc->getLocation(), diag::warn_sycl_pass_by_reference_future);


Note for other reviewers: since these are just warnings, we don't want them invalidating the kernelFunc.

clang/lib/Driver/ToolChains/Clang.cpp

bader · 2020-08-13T08:29:19Z

clang/lib/Frontend/InitPreprocessor.cpp

+    else if (LangOpts.SYCLVersion == 2020)
+      Builder.defineMacro("CL_SYCL_LANGUAGE_VERSION", "2020");


From SYCL-2020 provisional specification.

"SYCL_LANGUAGE_VERSION substitutes an integer reflecting the version number and revision of the SYCL
language being supported by the implementation. The version of SYCL defined in this document will
have SYCL_LANGUAGE_VERSION substitute the integer 202001, composed with the general SYCL version
followed by 2 digits representing the revision number;"

As SYCL targets non-OpenCL API now, I suggest defining SYCL_LANGUAGE_VERSION for any SYCL version using "YEARREVISION" format and define CL_SYCL_LANGUAGE_VERSION=121 only for SYCL-1.2.1.

thanks. Done.

clang/test/Preprocessor/sycl-macro.cpp

clang/test/SemaSYCL/accessor-type-diagnostics.cpp

cperkinsintel · 2020-08-17T14:55:20Z

/summary:run

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel · 2020-08-17T21:31:50Z

rebased (again) -

bader · 2020-08-18T12:56:37Z

/summary:run

cperkinsintel · 2020-08-18T21:57:58Z

I have no idea what is going on with the Jenkins Summary - the error doesn't even look like it's pulling the same PR. But as that is not required, is it possible to get this merged anyway?

This PR touches a lot of files, has been reviewed several times, but lately each time we go to merge there is a conflict with some other change, so I have to rebase, re-run the tests, re-request reviews, and by the time it all finishes, another conflict will be introduced.

Support _for_ `sycl-std=2020` was added in an earlier PR( #2259 ) but the default SYCL version was left alone. Now we are changing the FE so that if not otherwise specified `sycl-std=2020` is elected as the default. Signed-off-by: Chris Perkins <chris.perkins@intel.com>

SPIRVEntry created in function getEntry isn't deleted when it is ExtOp and isn't SourceContinued. Original commit: KhronosGroup/SPIRV-LLVM-Translator@b891a51

[L0] Enable Immediate Command Lists by default given Intel GPU DG2 or newer

cperkinsintel requested review from bader and romanovvlad August 4, 2020 23:39

cperkinsintel requested review from elizabethandrews, Fznamznon, premanandrao and a team as code owners August 4, 2020 23:39

cperkinsintel commented Aug 4, 2020

View reviewed changes

sycl/include/CL/sycl/queue.hpp Show resolved Hide resolved

cperkinsintel requested review from AGindinson and mdtoguchi as code owners August 5, 2020 01:25

cperkinsintel commented Aug 5, 2020

View reviewed changes

sycl/include/CL/sycl/queue.hpp Outdated Show resolved Hide resolved

cperkinsintel changed the title ~~[WIP] - Do Not Merge -- immutable kernel functions redo.~~ [SYCL] - Ready for Review -- immutable kernel functions redo. Aug 5, 2020

cperkinsintel force-pushed the cperkins-immutable-kernel-functions-redo branch from c2f532c to 2b005ed Compare August 5, 2020 20:06

romanovvlad reviewed Aug 10, 2020

View reviewed changes

sycl/include/CL/sycl/handler.hpp Show resolved Hide resolved

Fznamznon mentioned this pull request Aug 10, 2020

[SYCL] disallow mutable lambdas #1785

Merged

Fznamznon previously approved these changes Aug 10, 2020

View reviewed changes

erichkeane mentioned this pull request Aug 10, 2020

[SYCL]Move kernel diagnostics to the call, enable template trail on errors #2289

Merged

bader reviewed Aug 10, 2020

View reviewed changes

cperkinsintel dismissed Fznamznon’s stale review via b6abab4 August 12, 2020 21:40

cperkinsintel force-pushed the cperkins-immutable-kernel-functions-redo branch from c995f53 to b6abab4 Compare August 12, 2020 21:40

cperkinsintel requested review from bader and Fznamznon August 12, 2020 23:15

erichkeane reviewed Aug 12, 2020

View reviewed changes

erichkeane previously approved these changes Aug 13, 2020

View reviewed changes

erichkeane reviewed Aug 13, 2020

View reviewed changes

mdtoguchi reviewed Aug 13, 2020

View reviewed changes

clang/lib/Driver/ToolChains/Clang.cpp Outdated Show resolved Hide resolved

bader reviewed Aug 13, 2020

View reviewed changes

bader requested a review from romanovvlad August 13, 2020 12:11

cperkinsintel dismissed erichkeane’s stale review via 6fa753f August 13, 2020 15:53

cperkinsintel force-pushed the cperkins-immutable-kernel-functions-redo branch from 7ccef1e to 07fab10 Compare August 13, 2020 17:52

cperkinsintel added 17 commits August 17, 2020 13:28

immutable functions redo. WIP.

694bbcb

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

hooked up -sycl-std flag to set SYCL version constant. Needs test.

d68ba91

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

simplified macros - tricky re-use not worth it.

8390646

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

test added

f5df1d5

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

last changes, clang-formattery.

02dcc87

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang-formatota

b0cffd8

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

rebased to remove conflicts

7d43180

new test needed flag

daf304c

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

rebased, incorporated latest PR feedback

388a6db

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

overlooked test.

6bace1e

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang-format is grumpy

dc5863d

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

move conformance check

bbf2f95

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

hmm - git clang-format is skipping certain whitespace changes.

d84c1e9

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

removed unused var. stray from earlier change

0f9fe01

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

latest review requests

b87d642

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang-format, of course

728dd45

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

new SYCL_LANGUAGE_VERSION macro and etc.

391ba54

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel dismissed bader’s stale review via 391ba54 August 17, 2020 21:23

cperkinsintel force-pushed the cperkins-immutable-kernel-functions-redo branch from 8073b8e to 391ba54 Compare August 17, 2020 21:23

cperkinsintel requested a review from bader August 17, 2020 21:32

bader merged commit 1dbc358 into intel:sycl Aug 19, 2020

cperkinsintel mentioned this pull request Aug 28, 2020

[SYCL][Driver] Change default SYCL version to be 2020 #2386

Merged

cperkinsintel deleted the cperkins-immutable-kernel-functions-redo branch September 13, 2023 17:08

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2259 from nrspruit/immediate_dg2_or_newer_default

d04dbc2

[L0] Enable Immediate Command Lists by default given Intel GPU DG2 or newer

	// expected-warning@+2 {{Passing of kernel functions by reference is a SYCL 2020 extension}}
	// expected-warning@+2 {{Passing of kernel functions by reference is a SYCL 2020 extension}}

		else if (LangOpts.SYCLVersion == 2020)
		Builder.defineMacro("CL_SYCL_LANGUAGE_VERSION", "2020");

[SYCL] Immutable kernel functions redo. #2259

[SYCL] Immutable kernel functions redo. #2259

Uh oh!

Conversation

cperkinsintel commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cperkinsintel commented Aug 6, 2020

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichkeane Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

cperkinsintel Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

cperkinsintel Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cperkinsintel commented Aug 17, 2020

Uh oh!

cperkinsintel commented Aug 17, 2020

Uh oh!

bader commented Aug 18, 2020

Uh oh!

cperkinsintel commented Aug 18, 2020

Uh oh!

Uh oh!

cperkinsintel commented Aug 4, 2020 •

edited

Loading