[SYCL] Error for implicit capture of this pointer inside a sycl kernel #2029

asudarsa · 2020-07-02T15:01:25Z

Change-Id: Icf4145a3cf3732a0ff28f0ada9880a24b97b5a0d
Signed-off-by: Arvind Sudarsanam [email protected]

Change-Id: Icf4145a3cf3732a0ff28f0ada9880a24b97b5a0d Signed-off-by: Arvind Sudarsanam <[email protected]>

Signed-off-by: Arvind Sudarsanam <[email protected]>

Fznamznon · 2020-07-02T16:42:43Z

clang/lib/Sema/SemaSYCL.cpp

+  if (KernelLambda->isLambda()) {
+    llvm::DenseMap<const VarDecl *, FieldDecl *> Captures;
+    FieldDecl *ThisCapture;
+    KernelLambda->getCaptureFields(Captures, ThisCapture);
+    for (const LambdaCapture &LC : KernelLambda->captures()) {
+      if (LC.capturesThis() && LC.isImplicit()) {
+        Diag(LC.getLocation(), diag::err_implicit_this_capture);
+      }
+    }
+  }


Eeh, this is a bit outside of the whole Visitor model that we have in SemaSYCL. We often use SyclKernelFieldChecker to check kernel fields. The problem that it actually checks fields not captures. Right now, I don't know how to integrate it into the current visitor model, though.
@elizabethandrews , @erichkeane , any ideas?

Yeah, theres not really a good way to do this with the field checker. If this is a limitation in the standard, it is definitely something that should be diagnosed.

However, this message is wrong I think, this also looks like it will diagnose on capture with '&' of this as well.

Also, 1656,57, and 58 are unnecessary as far as I can tell, those lines likely need to be removed.

Actually SYCL 1.2.1 standard prohibits all sorts of passing pointers from host to device, however we support USM extension that actually allows some pointers to be passed to the kernel, but this pointer is definitely not the case of USM pointer and accidental capture of this pointer is very common in user's code and always produces fails in run time. This definitely should be diagnosed.

Actually SYCL 1.2.1 standard prohibits all sorts of passing pointers from host to device, however we support USM extension that actually allows some pointers to be passed to the kernel, but this pointer is definitely not the case of USM pointer and accidental capture of this pointer is very common in user's code and always produces fails in run time. This definitely should be diagnosed.

I think this should be an error only if strict compliance to SYCL 1.2.1 is requested; if USM is enabled, it should be a warning instead.

My understanding is that if a class is allocated in USM, the this pointer is a USM pointer and can be safely used in kernels.

Hi @Pennycook , Thanks for the comment. I agree that we should deal with USM pointers as a special case. Is there a way that the compiler can know if USM is enabled? Also, for a given class object, is it possible for the compiler to check if it is allocated in USM? I tried looking around in compiler sources and I did not see any other instance where we are checking for USM. May be I missed something.

@erichkeane. Thanks for your comment. I will remove 1656-58. I was using them earlier. Also, I will check to see if using [&] results in similar runtime error and update the error message accordingly.

Is there a way that the compiler can know if USM is enabled?

Adding @jbrodman and @AlexeySachkov. I seem to remember talk of providing something like an -fsycl-usm flag to control the USM extension, but I don't know what it's implementation status is.

Also, for a given class object, is it possible for the compiler to check if it is allocated in USM?

Unfortunately not. Whether the object was allocated in USM or not is something that can only be determined at runtime. This is why I suggested that we generate a warning in this case; a user would then see at compile-time that what they're doing may be unsafe, but would be free to ignore the warning if they wanted.

Alternatively, if we decide to stick with an error for implicit capture of this in all situations, we could display different error messages. If USM is disabled, capture of this is not allowed for SYCL kernels. If USM is enabled, capture of this is only allowed if the class was allocated in USM, and the user should explicitly specify this in the capture list if it was intended.

Hi @Pennycook
Thanks so much for the detailed feedback. I agree that we can emit two different error messages. But, I think that can be captured as a separate task. Please advise.

I think 'strict-sycl' (or sycl-strict?) was intended for this type of extension. I think we intended to have pointers in general handled with that flag.

In 'strict' SYCL mode, ALL captures of pointers (including 'this') is supposed to be ill-formed, by my understanding, so the extra rule of 'implicit capture of this' seems unnecessary*. That said, having our USM extension require explicit capture seems like a good idea, accidental capture of 'this' is unfortunately quite easy.

*The exception of course is capture via '=', where someone who doesn't understand how implicit this capture works would be burned. Since '=' means "take the state as it is now", accidental capture of 'this' is particularly surprising/dangerous.

Yes, accidental capture of this through [=] has caused a lot of pain.

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-07-02T17:54:58Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+  def err_implicit_this_capture : Error<
+    "implicit capture of 'this' with a capture default of '=' is not allowed "
+    "for kernel functions in SYCL 1.2.1">;


I'm not sure that we need to mention = in this diagnostic at all, because AFAIK SYCL kernel lambdas can be defined only with =.

Do we prohibit & capture? I realize that we can't capture references (right?!), but & capture of 'this' as a slightly different meaning.

We diagnose captured references (I mean the fields of kernel object that have reference type), but I don't remember diagnostic for the whole & lambda capture. Isn't it covered by captured references diagnostic? Should we diagnose it separately?

THAT covers it for the most part (thought it might be confusing since adding a line to a lambda will cause it to actually capture by reference), but capturing 'this' is a confusing story. Capturing 'this' by reference is exactly the same as not capturing by this, since the pointer cannot change.

We likely want to change the error message to say 'implicit capture of this', which is what the rest of the patch implements.

Is there a reason only implicit capture of this is not allowed? I would assume whatever issues arise from capturing this would exist for implicit and explicit capture?

Good question. I somehow was under impression that kernel lambdas can use only implicit capture, however I've opened the spec where it describes how to write kernel lambda and it says (pt. 4.8.9.2):

The kernel lambda must use copy for all of its captures (i.e. [=]).

For me this statement doesn't prohibit copy for explicit captures.

Yes, there is no reason to forbid explicit captures.

Fznamznon · 2020-07-02T17:56:01Z

clang/test/SemaSYCL/lambda_implicit_capture_this.cpp

+  kernel<class kernel_wrapper>(
+      [=]() {
+        int acc[1] = {5};
+        acc[0] *= member; // expected-error{{implicit capture of 'this' with a capture default of '=' is not allowed for kernel functions in SYCL 1.2.1}}


I spent some time to actually understand how this was actually captured. Can we also add a clarifying note?

Hi @Fznamznon , Thanks for the comment. Can you please let me know what we can mention in the clarifying note? Thanks.

I wanted some note like
'this' is captured through usage of member field inside the lambda
Although, I don't see a good way to implement it now, so, you can skip this comment.

asudarsa · 2020-07-09T16:24:24Z

I am looking at the failures now. In one of the existing test cases, a member function of a class is called inside the kernel and the compiler reports 'my' error message. I think this is correct behavior. I can update the test to have explicit capture of 'this' pointer and check if it passes after that.
I am currently running into some local testing issue which I have reported and will submit a new patch once I can test it. Thanks

Signed-off-by: Arvind Sudarsanam <[email protected]>

sycl/include/CL/sycl/handler.hpp

bader · 2020-07-14T18:33:56Z

clang/test/SemaSYCL/lambda_implicit_capture_this.cpp

@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -I %S/Inputs -fsycl -fsycl-is-device -verify %s


Suggested change

// RUN: %clang_cc1 -I %S/Inputs -fsycl -fsycl-is-device -verify %s

// RUN: %clang_cc1 -I %S/Inputs -fsycl -fsycl-is-device -fsyntax-only -verify %s

bader · 2020-07-14T18:35:04Z

clang/test/SemaSYCL/lambda_implicit_capture_this.cpp

@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -I %S/Inputs -fsycl -fsycl-is-device -verify %s
+
+#include <sycl.hpp>


Suggested change

#include <sycl.hpp>

Is this include really needed?

Signed-off-by: Arvind Sudarsanam <[email protected]>

sycl/include/CL/sycl/handler.hpp

elizabethandrews

LGTM except for diagnostic wording. @erichkeane can you also take a look?

clang/include/clang/Basic/DiagnosticSemaKinds.td

Signed-off-by: Arvind Sudarsanam <[email protected]>

erichkeane

1 coding-standard nit. Otherwise LGTM.

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Arvind Sudarsanam <[email protected]>

erichkeane · 2020-07-21T17:44:13Z

As a note for the future, we likely want to extract tests like this (that check the lambda itself) into a separate function. I believe @cperkinsintel has another patch that is a good candidate for this sort of thing, so when he goes to do that he likely wants to extract ALL lambda checking into its own function.

rbegam

LGTM

Error for implicit capture of this pointer inside a sycl kernel

393db1e

Change-Id: Icf4145a3cf3732a0ff28f0ada9880a24b97b5a0d Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa requested review from elizabethandrews, Fznamznon and premanandrao as code owners July 2, 2020 15:01

asudarsa requested a review from Pennycook July 2, 2020 15:11

asudarsa assigned asudarsa and unassigned asudarsa Jul 2, 2020

fixing a minor formatting issue

d86da94

Signed-off-by: Arvind Sudarsanam <[email protected]>

Fznamznon reviewed Jul 2, 2020

View reviewed changes

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

Fznamznon reviewed Jul 2, 2020

View reviewed changes

Incorporating code review comments and test failure fix

8ec2400

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa requested a review from a team as a code owner July 14, 2020 16:18

asudarsa requested a review from rbegam July 14, 2020 16:18

asudarsa added 2 commits July 14, 2020 09:23

Resolving merge conflicts

c6ca0b3

Signed-off-by: Arvind Sudarsanam <[email protected]>

Variable name changed

f52424e

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa requested review from bader and againull July 14, 2020 18:26

asudarsa commented Jul 14, 2020

View reviewed changes

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

bader reviewed Jul 14, 2020

View reviewed changes

asudarsa added 2 commits July 20, 2020 07:08

updating testcase based on comments

619cf1b

Signed-off-by: Arvind Sudarsanam <[email protected]>

resolving merge conflicts

a5811fa

Signed-off-by: Arvind Sudarsanam <[email protected]>

againull previously approved these changes Jul 20, 2020

View reviewed changes

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

elizabethandrews reviewed Jul 21, 2020

View reviewed changes

clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

updating error message

cb5840c

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa dismissed againull’s stale review via cb5840c July 21, 2020 17:07

erichkeane reviewed Jul 21, 2020

View reviewed changes

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

complying to llvm coding standards

bec8fd0

Signed-off-by: Arvind Sudarsanam <[email protected]>

erichkeane approved these changes Jul 21, 2020

View reviewed changes

elizabethandrews approved these changes Jul 21, 2020

View reviewed changes

rbegam approved these changes Jul 21, 2020

View reviewed changes

bader merged commit 1b9f026 into intel:sycl Jul 22, 2020

Pennycook mentioned this pull request Jul 22, 2020

Implicit capture of this pointer via [=] uxlfoundation/oneAPI-spec#127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Error for implicit capture of this pointer inside a sycl kernel #2029

[SYCL] Error for implicit capture of this pointer inside a sycl kernel #2029

asudarsa commented Jul 2, 2020

Fznamznon Jul 2, 2020

erichkeane Jul 2, 2020

Fznamznon Jul 2, 2020

Pennycook Jul 2, 2020

asudarsa Jul 9, 2020

asudarsa Jul 9, 2020

Pennycook Jul 10, 2020

asudarsa Jul 10, 2020

erichkeane Jul 10, 2020

jbrodman Jul 10, 2020

Fznamznon Jul 2, 2020

erichkeane Jul 2, 2020

Fznamznon Jul 2, 2020

erichkeane Jul 2, 2020

elizabethandrews Jul 3, 2020

Fznamznon Jul 6, 2020

keryell Jul 7, 2020

Fznamznon Jul 2, 2020

asudarsa Jul 9, 2020

Fznamznon Jul 10, 2020

asudarsa commented Jul 9, 2020

bader Jul 14, 2020

bader Jul 14, 2020

elizabethandrews left a comment

erichkeane left a comment

erichkeane commented Jul 21, 2020

rbegam left a comment

		@@ -0,0 +1,28 @@
		// RUN: %clang_cc1 -I %S/Inputs -fsycl -fsycl-is-device -verify %s

		@@ -0,0 +1,28 @@
		// RUN: %clang_cc1 -I %S/Inputs -fsycl -fsycl-is-device -verify %s

		#include <sycl.hpp>

[SYCL] Error for implicit capture of this pointer inside a sycl kernel #2029

[SYCL] Error for implicit capture of this pointer inside a sycl kernel #2029

Conversation

asudarsa commented Jul 2, 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

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

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

asudarsa commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane commented Jul 21, 2020

rbegam left a comment

Choose a reason for hiding this comment