[SYCL] disallow mutable lambdas #1785

cperkinsintel · 2020-05-29T21:44:25Z

With the latest changes in the SYCL spec kernel lambdas (and functors) must be const references. Have updated the SYCL headers and tests with this change. Also had to make a minor adjustment to the kernel generation code in Clang and update many of the SemaSYCL tests, because their mock kernel defs needed to be updated as well.

Signed-off-by: Chris Perkins [email protected]

AlexeySachkov · 2020-05-31T20:29:24Z

I did not update the definitions of the various reductions. Should I do so?

@vzakhari, @Pennycook: could you please answer here?

vzakhari · 2020-06-01T21:19:31Z

I did not update the definitions of the various reductions. Should I do so?

@vzakhari, @Pennycook: could you please answer here?

@AlexeySachkov, did you want to contact @v-klochkov instead of me?

Pennycook · 2020-06-01T21:27:43Z

I did not update the definitions of the various reductions. Should I do so?

@vzakhari, @Pennycook: could you please answer here?

I think the interfaces should be consistent. If parallel_for shouldn't accept mutable lambdas, the form of parallel_for with support for reductions shouldn't accept mutable lambdas either.

romanovvlad · 2020-07-08T12:18:10Z

@cperkinsintel Please, click on "ready for review" button once it's ready for review.

sycl/include/CL/sycl/handler.hpp

v-klochkov · 2020-07-20T22:03:28Z

@v-klochkov v-klochkov requested review from elizabethandrews, Fznamznon and premanandrao as code owners now

It the result of mis-click (accidentally clicked 'Ready for review' button, which did not ask for any confirmations after the click).
It still may be useful for code-owners to start reviewing this patch.

erichkeane

I don't think changing all the tests in the CFE is the right idea. While I am fine fixing a bunch of them, we should still ensure the old behavior still works. This many file changes is likely unnecessary and just distracting review-wise.

Additionally, I think we should warn in both cases based on the SYCL version. So in 1.2.1, we should warn like we do with other language compatibility items (such as allowing C++11 features in 03, etc). In the newer one, we should warn to change it.

erichkeane · 2020-07-20T23:00:37Z

clang/lib/Sema/SemaSYCL.cpp

@@ -730,7 +730,13 @@ getKernelInvocationKind(FunctionDecl *KernelCallerFunc) {
 }

 static CXXRecordDecl *getKernelObjectType(FunctionDecl *Caller) {
-  return (*Caller->param_begin())->getType()->getAsCXXRecordDecl();
+  auto KernelParam = (*Caller->param_begin());


Suggested change

auto KernelParam = (*Caller->param_begin());

auto KernelParamTy = (*Caller->param_begin())->getType();

thanks for catching that. Done.

clang/lib/Sema/SemaSYCL.cpp

cperkinsintel · 2020-07-21T00:03:36Z

@erichkeane - There is a test that ensures that the SYCL 1.2 and SYCL 2020 versions are both supported: clang/test/CodeGenSYCL/kernel-by-reference.cpp

Also I agree about emitting a warning. But that seems out-of-scope for this change. How does the clang front end want to handle different versions of the SYCL runtime headers? Is this the only place we'll encounter differences? I think opening a ticket for it might be better.

erichkeane · 2020-07-21T12:50:09Z

@erichkeane - There is a test that ensures that the SYCL 1.2 and SYCL 2020 versions are both supported: clang/test/CodeGenSYCL/kernel-by-reference.cpp

Also I agree about emitting a warning. But that seems out-of-scope for this change. How does the clang front end want to handle different versions of the SYCL runtime headers? Is this the only place we'll encounter differences? I think opening a ticket for it might be better.

I would think it is exactly in scope of this change, but I'll leave that to the code owners to make that decision.

Since we have a test that makes sure both works, I don't really see much value in changing the other 100+ tests, but I'll again leave that to the code owners to decide.

This adds const-correctness to a bit of the visitor, as brought up in review on intel#1785.

This adds const-correctness to a bit of the visitor, as brought up in review on #1785.

romanovvlad

I'm OK with the runtime part.

cperkinsintel · 2020-07-22T21:30:22Z

@Fznamznon @elizabethandrews @premanandrao as code owners of the front-end, could one of you review that side of it? I had to update all those ersatz test kernels to bring them up to date with the new const-ref requirement on their lambdas. That touches a lot files, but the change is trivial, just avoids deferred maintenance. Other than that, the FE changes are very small.

premanandrao · 2020-07-23T13:59:56Z

@Fznamznon @elizabethandrews @premanandrao as code owners of the front-end, could one of you review that side of it? I had to update all those ersatz test kernels to bring them up to date with the new const-ref requirement on their lambdas. That touches a lot files, but the change is trivial, just avoids deferred maintenance. Other than that, the FE changes are very small.

I am fine with the source code changes; it is just the volume of test changes that I was uncomfortable with.
What is the long-term plan for SYCL 1.2.1 way of passing lambdas? Will we continue to support it for a long time? If so, I am a bitconcerned about the lack of test coverage for that style. On the other hand, if the old style will be disallowed in the near future, then changing all the tests and having one test for completeness seems okay.

Signed-off-by: Chris Perkins <[email protected]>

cperkinsintel · 2020-07-24T18:06:40Z

I've rebased, updated the conflicted test, and incorporated in @erichkeane latest request. Should be good to go.

clang/lib/Sema/SemaSYCL.cpp

… generate some more discussion Signed-off-by: Chris Perkins <[email protected]>

Signed-off-by: Chris Perkins <[email protected]>

cperkinsintel · 2020-07-28T20:16:43Z

@erichkeane - Ok, I've added the second diagnostic, moved the emitting of it to its own function called from ConstructOpenCLKernel, put the warnings in their own groups, -Wno suppressed that same warning in a bunch of the tests that are now using the 2020 kernel definitions. Let me know if there are other changes.

clang/include/clang/Basic/DiagnosticSemaKinds.td

erichkeane · 2020-07-28T20:44:52Z

clang/include/clang/Basic/DiagnosticGroups.td

@@ -1127,7 +1127,9 @@ def OpenMP : DiagGroup<"openmp", [
  ]>;

 // SYCL warnings
-def SyclStrict : DiagGroup<"sycl-strict">;
+def Sycl2017Conform : DiagGroup<"sycl-2017-conform">;


perhaps sycl-2017-compat to be more consistent with c++98-compat, et al?

yes, good call. Done.

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-07-28T20:48:38Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1948,6 +1971,7 @@ void Sema::ConstructOpenCLKernel(FunctionDecl *KernelCallerFunc,
      if (LC.capturesThis() && LC.isImplicit())
        Diag(LC.getLocation(), diag::err_implicit_this_capture);
  }
+  diagKernelCallerSpecConformance(*this, KernelCallerFunc);


The comment I had made in the past was to move the checks from 1969 into their own function as soon as we had a 2nd thing to check. So please make 1 function that checks both:

checkKernel? Anyone wanna bikeshed for me? @elizabethandrews @premanandrao ?

checkKernelParameters?

checkKernelAndCaller ?

checkKernelCompat ?

checkKernelParameters?

Its not really the Parameters, to the kernel, its the kernel object itself (plus the caller).

checkKernelAndCaller ?
Probably good enough for now. Presumably someone will come up with a better idea in the future and we can rename it.

Signed-off-by: Chris Perkins <[email protected]>

erichkeane

2 nits, otherwise LGTM.

clang/include/clang/Basic/DiagnosticSemaKinds.td

erichkeane · 2020-07-28T23:01:50Z

clang/lib/Sema/SemaSYCL.cpp

+  }
+
+  // check that calling kernel conforms to spec
+  QualType KernelParamTy = (*Caller->param_begin())->getType();


Probably want an assert here that Caller->param_size() >=1.

Signed-off-by: Chris Perkins <[email protected]>

This reverts commit a43dcc2.

…rogram * upstream/sycl: (609 commits) [SYCL] Fix fail in the post commit testing (intel#2210) [SYCL] Materialize shadow local variables for byval arguments before use (intel#2200) [SYCL] Support lambda functions passed to reduction (intel#2190) [SYCL][USM] Improve USM Allocator. (intel#2026) [SYCL] Disallow mutable lambdas (intel#1785) [SYCL][ESIMD] Setup compilation pipeline for ESIMD (intel#2134) [SYCL] Fix not found kernel due to empty kernel name when using set_arg(s) (intel#2181) [SYCL] Fixed check for set_arg (intel#2203) Refactor indirect access calls to minimize invocations. (intel#2185) [SYCL][NFC] Fix potential null-pointer access (intel#2197) [SYCL] Propagate attributes from transitive calls to kernel (intel#1878) [SYCL] Fix warnings from static analysis tool (intel#2193) [SYCL][NFC] Fix ac_float test for compilation with FE optimizations (intel#2184) [GitHub Actions] Uplift clang-format version to 10 (intel#2194) [SYCL][ESIMD] Pass to replace simd* parameters with native llvm vectors. (intel#2097) [SYCL][NFC] Fixed SYCL_PI_TRACE output while selecting a device. (intel#2192) [SYCL][FPGA] New spec for controlling load-store units in FPGAs (intel#2158) [SYCL][Doc] Clarify reqd_sub_group_size (intel#2103) [SYCL] Remove noreturn function attribute (intel#2165) [SYCL] Aligned set_arg behaviour with SYCL specification (intel#2159) ...

This reverts commit a43dcc2.

…rogram * upstream/sycl: [SYCL] Add barrier before leader guard in LowerWGSCope pass (intel#2208) [SYCL] Add prototype of atomic_ref<T*> (intel#2177) Revert "[SYCL] Disallow mutable lambdas (intel#1785)" (intel#2213)

erichkeane · 2020-08-10T14:22:23Z

@cperkinsintel : This got reverted, so you need to update this to figure out why it caused a bunch of test failures.

Fznamznon · 2020-08-10T14:25:43Z

@cperkinsintel : This got reverted, so you need to update this to figure out why it caused a bunch of test failures.

I guess he already did, see #2259

erichkeane · 2020-08-10T14:27:12Z

Ah, i see! He didn't mention this PR# so I guess it didn't get picked up.

cperkinsintel requested review from romanovvlad and mkinsner May 29, 2020 21:44

cperkinsintel requested a review from a team as a code owner May 29, 2020 21:44

cperkinsintel changed the title ~~disallow mutable lambdas~~ [SYCL] disallow mutable lambdas May 30, 2020

romanovvlad marked this pull request as draft July 8, 2020 12:17

cperkinsintel force-pushed the cperkins-no-mutable-kernel-func branch from 1eada98 to 15c522e Compare July 14, 2020 21:05

romanovvlad requested changes Jul 15, 2020

View reviewed changes

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

cperkinsintel requested a review from v-klochkov July 15, 2020 16:03

v-klochkov reviewed Jul 16, 2020

View reviewed changes

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

cperkinsintel force-pushed the cperkins-no-mutable-kernel-func branch from 7887e98 to 52a534d Compare July 18, 2020 06:07

v-klochkov marked this pull request as ready for review July 20, 2020 22:00

v-klochkov requested review from elizabethandrews, Fznamznon and premanandrao as code owners July 20, 2020 22:00

cperkinsintel requested a review from erichkeane July 20, 2020 22:13

erichkeane reviewed Jul 20, 2020

View reviewed changes

erichkeane pushed a commit to erichkeane/llvm that referenced this pull request Jul 21, 2020

[NFC][SYCL] Add const to some uses of CXXRecordDecl

2a22824

This adds const-correctness to a bit of the visitor, as brought up in review on intel#1785.

erichkeane mentioned this pull request Jul 21, 2020

[NFC][SYCL] Add const to some uses of CXXRecordDecl #2153

Merged

bader pushed a commit that referenced this pull request Jul 21, 2020

[NFC][SYCL] Add const to some uses of CXXRecordDecl (#2153)

c7f397a

This adds const-correctness to a bit of the visitor, as brought up in review on #1785.

cperkinsintel force-pushed the cperkins-no-mutable-kernel-func branch from 02d8f7c to 29ffe63 Compare July 21, 2020 21:42

romanovvlad previously approved these changes Jul 22, 2020

View reviewed changes

cperkinsintel added 4 commits July 24, 2020 10:30

rebase (again). no more const casting

750aa5f

Signed-off-by: Chris Perkins <[email protected]>

clang-formatf

bc3bdb3

Signed-off-by: Chris Perkins <[email protected]>

clang coding guidelines

c697445

Signed-off-by: Chris Perkins <[email protected]>

rebase and update test

000286a

Signed-off-by: Chris Perkins <[email protected]>

cperkinsintel dismissed romanovvlad’s stale review via 000286a July 24, 2020 18:02

cperkinsintel force-pushed the cperkins-no-mutable-kernel-func branch from bfd5f8b to 000286a Compare July 24, 2020 18:02

erichkeane reviewed Jul 24, 2020

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

cperkinsintel added 4 commits July 24, 2020 16:06

adding warning diagnostic when sycl 1.2 encountered. Expect this will…

c87f7b7

… generate some more discussion Signed-off-by: Chris Perkins <[email protected]>

updated diagnostics - sycl version aware, diff messaging

ed43c1b

Signed-off-by: Chris Perkins <[email protected]>

second diagnostic, reorg, grouping of warnings.

aa6423c

Signed-off-by: Chris Perkins <[email protected]>

updated tests with flag to suppress conformance warning

64b8d7e

Signed-off-by: Chris Perkins <[email protected]>

erichkeane reviewed Jul 28, 2020

View reviewed changes

feedback from review

29e916f

Signed-off-by: Chris Perkins <[email protected]>

erichkeane reviewed Jul 28, 2020

View reviewed changes

nits

ab280a4

Signed-off-by: Chris Perkins <[email protected]>

erichkeane approved these changes Jul 29, 2020

View reviewed changes

premanandrao approved these changes Jul 29, 2020

View reviewed changes

romanovvlad approved these changes Jul 29, 2020

View reviewed changes

romanovvlad merged commit a43dcc2 into intel:sycl Jul 29, 2020

romanovvlad added a commit that referenced this pull request Jul 29, 2020

Revert "[SYCL] Disallow mutable lambdas (#1785)"

ffd8794

This reverts commit a43dcc2.

romanovvlad mentioned this pull request Jul 29, 2020

Revert "[SYCL] disallow mutable lambdas" #2213

Merged

vladimirlaz pushed a commit that referenced this pull request Jul 30, 2020

Revert "[SYCL] Disallow mutable lambdas (#1785)" (#2213)

8ac87a3

This reverts commit a43dcc2.

cperkinsintel deleted the cperkins-no-mutable-kernel-func branch September 13, 2023 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] disallow mutable lambdas #1785

[SYCL] disallow mutable lambdas #1785

cperkinsintel commented May 29, 2020 •

edited

Loading

AlexeySachkov commented May 31, 2020

vzakhari commented Jun 1, 2020

Pennycook commented Jun 1, 2020

romanovvlad commented Jul 8, 2020

v-klochkov commented Jul 20, 2020

erichkeane left a comment

erichkeane Jul 20, 2020

cperkinsintel Jul 20, 2020

cperkinsintel commented Jul 21, 2020

erichkeane commented Jul 21, 2020

romanovvlad left a comment

cperkinsintel commented Jul 22, 2020

premanandrao commented Jul 23, 2020

cperkinsintel commented Jul 24, 2020

cperkinsintel commented Jul 28, 2020

erichkeane Jul 28, 2020

cperkinsintel Jul 28, 2020

erichkeane Jul 28, 2020

premanandrao Jul 28, 2020

cperkinsintel Jul 28, 2020

cperkinsintel Jul 28, 2020

erichkeane Jul 28, 2020

cperkinsintel Jul 28, 2020

erichkeane left a comment

erichkeane Jul 28, 2020

cperkinsintel Jul 28, 2020

erichkeane commented Aug 10, 2020

Fznamznon commented Aug 10, 2020

erichkeane commented Aug 10, 2020

	auto KernelParam = (*Caller->param_begin());
	auto KernelParamTy = (*Caller->param_begin())->getType();

[SYCL] disallow mutable lambdas #1785

[SYCL] disallow mutable lambdas #1785

Conversation

cperkinsintel commented May 29, 2020 • edited Loading

AlexeySachkov commented May 31, 2020

vzakhari commented Jun 1, 2020

Pennycook commented Jun 1, 2020

romanovvlad commented Jul 8, 2020

v-klochkov commented Jul 20, 2020

erichkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cperkinsintel commented Jul 21, 2020

erichkeane commented Jul 21, 2020

romanovvlad left a comment

Choose a reason for hiding this comment

cperkinsintel commented Jul 22, 2020

premanandrao commented Jul 23, 2020

cperkinsintel commented Jul 24, 2020

cperkinsintel commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichkeane commented Aug 10, 2020

Fznamznon commented Aug 10, 2020

erichkeane commented Aug 10, 2020

cperkinsintel commented May 29, 2020 •

edited

Loading