[SYCL] Emit a warning on constexprs captured in SYCL kernels #2824

srividya-sundaram · 2020-11-26T18:12:46Z

In some cases Clang FE captures constexprs variables into a lambda. This can cause performance problems in case of SYCL, because

All captured variables become a kernel parameter, which shouldn't be the case for constexprs
Captured constexpr will not appear as a compile-time constant to the compiler

This patch warns users that a constexpr has been captured as a kernel argument and is not a compile-time constant.
The exact wordings of the warning message is open for wordsmithing.

srividya-sundaram · 2020-11-30T19:55:20Z

clang format fails for warning-flags.c, but formatting it results in modifying the file in a weird way and also appends the RUN lines together. Ex: see here

clang/include/clang/Basic/DiagnosticGroups.td

clang/test/SemaSYCL/warn-constexpr-kernel-arg.cpp

clang/test/Misc/warning-flags.c

clang/lib/Sema/SemaSYCL.cpp

clang/include/clang/Basic/DiagnosticGroups.td

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/test/SemaSYCL/warn-constexpr-kernel-arg.cpp

srividya-sundaram · 2020-12-01T19:49:38Z

@bader The test failures reported are all related to sycl-post-link tool in /localdisk2/sycl_ci/buildbot/worker/sycl-ubu-x64-pr/llvm.src/llvm/test/tools/sycl-post-link/

I verified the tests locally and they all pass. Could you clarify if this a buildbot related issue?
Also, clang-format check has been queued for a long time now.

Fznamznon · 2020-12-01T20:11:15Z

@bader The test failures reported are all related to sycl-post-link tool in /localdisk2/sycl_ci/buildbot/worker/sycl-ubu-x64-pr/llvm.src/llvm/test/tools/sycl-post-link/

@AlexeySachkov , could you please take a look? Can it be caused by e481174 ?

AlexeySachkov · 2020-12-01T20:24:41Z

@bader The test failures reported are all related to sycl-post-link tool in /localdisk2/sycl_ci/buildbot/worker/sycl-ubu-x64-pr/llvm.src/llvm/test/tools/sycl-post-link/

@AlexeySachkov , could you please take a look? Can it be caused by e481174 ?

Sure, I will take a look. It seems like pulldown has landed before my PR got merged, but after I had launched pre-commit: there was a change in typed attributes for function arguments, I just need to re-generate LLVM IR for those tests

AlexeySachkov · 2020-12-01T20:45:16Z

@bader The test failures reported are all related to sycl-post-link tool in /localdisk2/sycl_ci/buildbot/worker/sycl-ubu-x64-pr/llvm.src/llvm/test/tools/sycl-post-link/

@AlexeySachkov , could you please take a look? Can it be caused by e481174 ?

Sure, I will take a look. It seems like pulldown has landed before my PR got merged, but after I had launched pre-commit: there was a change in typed attributes for function arguments, I just need to re-generate LLVM IR for those tests

Should be fixed by #2846

clang/include/clang/Basic/DiagnosticSemaKinds.td

elizabethandrews

Thanks!

srividya-sundaram · 2020-12-03T17:04:31Z

@bader This is ready to be merged.

bader · 2020-12-03T17:15:35Z

It sounds like the compiler drops constexpr semantics. Could you clarify why this is not a compiler bug, please?

srividya-sundaram · 2020-12-03T17:27:42Z

It sounds like the compiler drops constexpr semantics. Could you clarify why this is not a compiler bug, please?

Compiler captures a constexpr as a kernel argument only when constexpr is ODR-used inside the lambda.
In other cases, it is treated as a compile-time constant as it should be.
Since the compiler captures a constexpr value into the kernel lambda as a parameter only in some special cases (ODR-use of constexpr), this is treated as a warning, instead of an error.
Hope I understood your question correctly.

premanandrao · 2020-12-03T17:48:27Z

It sounds like the compiler drops constexpr semantics. Could you clarify why this is not a compiler bug, please?

The warning generated is only for implicit captures. Since the address of the constexpr variable is taken, it gets turned into a kernel parameter. Otherwise, it remains constexpr. @erichkeane says that this could potentially be addressed in the library with overloads (and that it would be difficult to do so), but I didn't quite follow that thought.

bader · 2020-12-04T08:48:38Z

clang/test/SemaSYCL/warn-constexpr-kernel-arg.cpp

+    h.single_task<LambdaKernel>(
+        [=]() {
+          // constexpr 'OdrUsedVar' is odr-used here.
+          const unsigned *ptr = &OdrUsedVar; // expected-warning {{captured constexpr 'OdrUsedVar' will be a kernel argument in device code}}


I don't quite get the purpose of this warning.

I think in order to understand this warning message, user must know DPC++ implementation details about lowering lambda object into kernel function and how kernel function parameters are composed. Do you think it's clear for DPC++ users?

Another point it's generic C++ behavior and not SYCL specific issue:

template <typename T> void foo(T t) {t();} // ... foo([=]() { const unsigned *ptr = &OdrUsedVar; }); // OdrUsedVar is captured and might cause performance issues foo([=]() { unsigned x = 10 + OdrUsedVar; }); // OdrUsedVar is not captured. No performance issues?

Should we emit similar warning for any lambda expression?

It's probably more useful to implement such diagnostics in performance profiler because passing additional argument might be not a problem at all.

@premanandrao, @srividya-sundaram, @erichkeane, what do you think?

tagging @kbobrovs to give his feedback as well since this was originally brought up by him.

One more thing: this diagnostics warns that current implementation is not optimal.
I assume if compiler detects odr-use of constexpr it can allocate a global constant/variable in device code and kernel function object can capture global object and avoid passing it via kernel argument. This seems to be valid implementation. Right?

It's probably more useful to implement such diagnostics in performance profiler because passing additional argument might be not a problem at all.

I suspect many people (like myself) writing code similar to

constexpr int V = 137; auto f = [=](Id i) -> int { return (i * V).x; };

will expect V to be compile-time constant inside the lambda. In some cases this expectation is not true and V is captured and hence is not a compile-time constant. This can potentially lead to performance problems.

I'm not sure what 'performance compiler' is. If it can be built from this github repo - I'm OK with making this feature available only in 'performance compiler'.

FWIW, that is a fairly well known property of constexpr: It isn't necessarily a compile time constant unless you make it so in some way. I'm generally not in favor of a warning that tells you that the language is still working the way it is designed, particularly since there isn't a code-way to suppress this warning.

If we had some sort of perf-analysis tool, that would be a good place for that. Have we considered seeing if the Static Analysis effort would be a better place for this?

kbobrovs · 2020-12-07T13:02:40Z

clang/test/SemaSYCL/warn-constexpr-kernel-arg.cpp

+    h.single_task<LambdaKernel>(
+        [=]() {
+          // constexpr 'OdrUsedVar' is odr-used here.
+          const unsigned *ptr = &OdrUsedVar; // expected-warning {{captured constexpr 'OdrUsedVar' will be a kernel argument in device code}}


It's probably more useful to implement such diagnostics in performance profiler because passing additional argument might be not a problem at all.

I suspect many people (like myself) writing code similar to

constexpr int V = 137; auto f = [=](Id i) -> int { return (i * V).x; };

will expect V to be compile-time constant inside the lambda. In some cases this expectation is not true and V is captured and hence is not a compile-time constant. This can potentially lead to performance problems.

I'm not sure what 'performance compiler' is. If it can be built from this github repo - I'm OK with making this feature available only in 'performance compiler'.

kbobrovs · 2020-12-07T13:04:14Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -7449,6 +7449,9 @@ let CategoryName = "Lambda Issue" in {
    "%select{| explicitly}1 captured here">;
  def err_implicit_this_capture : Error<
    "implicit capture of 'this' is not allowed for kernel functions">;
+  def warn_constexpr_kernel_arg : Warning<
+    "captured constexpr '%0' will be a kernel argument in device code">,


Suggested change

"captured constexpr '%0' will be a kernel argument in device code">,

"captured constexpr '%0' will *not* be a compile-time constant in the device code">,

erichkeane · 2020-12-07T15:25:37Z

I'm not in favor of putting a warning like this into the compiler. Warnings are supposed to be actionable, not informative. If there isn't a code-based way to suppress a warning (and get the behavior you want), they generally aren't put into the compiler.

The issue here is closer to a bug (though not quite?) in the library. We're implementing conversion operators that aren't constexpr, so they cause us to need to use the variable in an ODR way, and thus need to be captured. This is a 'feature' of the language.

IMO, if we want to warn on this, I'd suggest this go into the static analyzer.

kbobrovs · 2020-12-07T18:22:40Z

Warnings are supposed to be actionable, not informative.

Agree. Possible action is to replace with #define. Or try to figure out the construct causing this (the * operator) and restructure the code (less attractive to a developer).

This is a 'feature' of the language.

I see. But I wonder what share of SYCL developers know C++ well enough to have the right expectation of the result of the code here :) I think such warning would save many people some time and annoy just few experts. But I'll let you FE folks decide what's best here.

srividya-sundaram · 2020-12-09T18:35:03Z

Closing this PR since the above discussion suggests that this change should not be implemented in CFE.

OpAtomicCompareExchangeWeak has been removed and #2665 added a validation for it. This pull request replaces OpAtomicCompareExchangeWeak with OpAtomicCompareExchange. Original commit: KhronosGroup/SPIRV-LLVM-Translator@eb64e4d795006d5

srividya-sundaram added 4 commits November 26, 2020 10:08

[SYCL] Emit a warning on constexprs captured in SYCL kernels

43eca38

Add a new warning group

2f4908e

Undo clang-format on warning-flags.c

4b315e9

Add newline at EOF

e441366

srividya-sundaram marked this pull request as ready for review November 30, 2020 19:56

srividya-sundaram requested review from elizabethandrews, Fznamznon and premanandrao as code owners November 30, 2020 19:56

premanandrao reviewed Nov 30, 2020

View reviewed changes

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

clang/test/SemaSYCL/warn-constexpr-kernel-arg.cpp Outdated Show resolved Hide resolved

clang/test/Misc/warning-flags.c Outdated Show resolved Hide resolved

srividya-sundaram added 2 commits November 30, 2020 14:18

Update test

98043f9

List captured variable

004e0fa

Fznamznon reviewed Dec 1, 2020

View reviewed changes

elizabethandrews reviewed Dec 1, 2020

View reviewed changes

clang/test/SemaSYCL/warn-constexpr-kernel-arg.cpp Outdated Show resolved Hide resolved

Address code review comments

b1fb78b

Update group name

71a9085

bader requested review from elizabethandrews, premanandrao and Fznamznon December 2, 2020 07:49

Fznamznon reviewed Dec 2, 2020

View reviewed changes

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

Simplified test

1f39874

srividya-sundaram requested a review from Fznamznon December 2, 2020 20:21

Fznamznon approved these changes Dec 2, 2020

View reviewed changes

elizabethandrews approved these changes Dec 2, 2020

View reviewed changes

premanandrao approved these changes Dec 3, 2020

View reviewed changes

bader reviewed Dec 4, 2020

View reviewed changes

kbobrovs reviewed Dec 7, 2020

View reviewed changes

srividya-sundaram closed this Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Emit a warning on constexprs captured in SYCL kernels #2824

[SYCL] Emit a warning on constexprs captured in SYCL kernels #2824

srividya-sundaram commented Nov 26, 2020 •

edited

Loading

srividya-sundaram commented Nov 30, 2020 •

edited

Loading

srividya-sundaram commented Dec 1, 2020

Fznamznon commented Dec 1, 2020

AlexeySachkov commented Dec 1, 2020

AlexeySachkov commented Dec 1, 2020

elizabethandrews left a comment

srividya-sundaram commented Dec 3, 2020

bader commented Dec 3, 2020

srividya-sundaram commented Dec 3, 2020 •

edited

Loading

premanandrao commented Dec 3, 2020

bader Dec 4, 2020

srividya-sundaram Dec 4, 2020 •

edited

Loading

bader Dec 7, 2020

kbobrovs Dec 7, 2020

erichkeane Dec 7, 2020

kbobrovs Dec 7, 2020

kbobrovs Dec 7, 2020

erichkeane commented Dec 7, 2020

kbobrovs commented Dec 7, 2020

srividya-sundaram commented Dec 9, 2020

	"captured constexpr '%0' will be a kernel argument in device code">,
	"captured constexpr '%0' will not be a compile-time constant in the device code">,

[SYCL] Emit a warning on constexprs captured in SYCL kernels #2824

[SYCL] Emit a warning on constexprs captured in SYCL kernels #2824

Conversation

srividya-sundaram commented Nov 26, 2020 • edited Loading

srividya-sundaram commented Nov 30, 2020 • edited Loading

srividya-sundaram commented Dec 1, 2020

Fznamznon commented Dec 1, 2020

AlexeySachkov commented Dec 1, 2020

AlexeySachkov commented Dec 1, 2020

elizabethandrews left a comment

Choose a reason for hiding this comment

srividya-sundaram commented Dec 3, 2020

bader commented Dec 3, 2020

srividya-sundaram commented Dec 3, 2020 • edited Loading

premanandrao commented Dec 3, 2020

bader Dec 4, 2020

Choose a reason for hiding this comment

srividya-sundaram Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

bader Dec 7, 2020

Choose a reason for hiding this comment

kbobrovs Dec 7, 2020

Choose a reason for hiding this comment

erichkeane Dec 7, 2020

Choose a reason for hiding this comment

kbobrovs Dec 7, 2020

Choose a reason for hiding this comment

kbobrovs Dec 7, 2020

Choose a reason for hiding this comment

erichkeane commented Dec 7, 2020

kbobrovs commented Dec 7, 2020

srividya-sundaram commented Dec 9, 2020

srividya-sundaram commented Nov 26, 2020 •

edited

Loading

srividya-sundaram commented Nov 30, 2020 •

edited

Loading

srividya-sundaram commented Dec 3, 2020 •

edited

Loading

srividya-sundaram Dec 4, 2020 •

edited

Loading