Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
43eca38
[SYCL] Emit a warning on constexprs captured in SYCL kernels
srividya-sundaram Nov 26, 2020
2f4908e
Add a new warning group
srividya-sundaram Nov 30, 2020
4b315e9
Undo clang-format on warning-flags.c
srividya-sundaram Nov 30, 2020
e441366
Add newline at EOF
srividya-sundaram Nov 30, 2020
98043f9
Update test
srividya-sundaram Nov 30, 2020
004e0fa
List captured variable
srividya-sundaram Dec 1, 2020
b1fb78b
Address code review comments
srividya-sundaram Dec 1, 2020
71a9085
Update group name
srividya-sundaram Dec 1, 2020
1f39874
Simplified test
srividya-sundaram Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,3 +1252,7 @@ in addition with the pragmas or -fmax-tokens flag to get any warnings.
def WebAssemblyExceptionSpec : DiagGroup<"wasm-exception-spec">;

def RTTI : DiagGroup<"rtti">;

// A warning group for warnings related to non-obvious
// performance degradation.
def PerfImpact : DiagGroup<"perf-impact">;
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">,

InGroup<PerfImpact>;

// C++14 lambda init-captures.
def warn_cxx11_compat_init_capture : Warning<
Expand Down
18 changes: 13 additions & 5 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3039,11 +3039,19 @@ void Sema::CheckSYCLKernelCall(FunctionDecl *KernelFunc, SourceRange CallLoc,
}

if (KernelObj->isLambda()) {
for (const LambdaCapture &LC : KernelObj->captures())
if (LC.capturesThis() && LC.isImplicit()) {
Diag(LC.getLocation(), diag::err_implicit_this_capture);
Diag(CallLoc.getBegin(), diag::note_used_here);
KernelFunc->setInvalidDecl();
for (const LambdaCapture &Capture : KernelObj->captures())
if (Capture.isImplicit()) {
if (Capture.capturesThis()) {
Diag(Capture.getLocation(), diag::err_implicit_this_capture);
Diag(CallLoc.getBegin(), diag::note_used_here);
KernelFunc->setInvalidDecl();
}
// Emit a warning on constexprs captured in SYCL kernels
if (Capture.capturesVariable()) {
if (Capture.getCapturedVar()->isConstexpr())
Diag(Capture.getLocation(), diag::warn_constexpr_kernel_arg)
<< Capture.getCapturedVar()->getNameAsString();
}
}
}

Expand Down
27 changes: 27 additions & 0 deletions clang/test/SemaSYCL/warn-constexpr-kernel-arg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// RUN: %clang_cc1 -fsycl -fsycl-is-device -internal-isystem %S/Inputs -fsyntax-only -sycl-std=2020 -verify %s

// This test warns users when an ODR-use of a constexpr variable causes the kernel lambda to capture it as a
// kernel argument

#include "sycl.hpp"

using namespace cl::sycl;

queue q;

class LambdaKernel;

int main() {

constexpr unsigned OdrUsedVar = 10;

q.submit([&](handler &h) {
// expected-note@+1 {{in instantiation of function template specialization}}
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}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@srividya-sundaram srividya-sundaram Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

});
});
return 0;
}