Skip to content

[SYCL] Fix regression introduced by f537293/part of #1018 fix. #1019

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

Merged
merged 2 commits into from
Jan 17, 2020
Merged

[SYCL] Fix regression introduced by f537293/part of #1018 fix. #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b069868
[SYCL] Fix regression introduced by f537293/part of #1018 fix.
Jan 15, 2020
120595a
[SYCL] Add test to validate the call stack, as Mariya requested.
Jan 17, 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
6 changes: 6 additions & 0 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4458,6 +4458,12 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,

if (FD)
diagnoseArgDependentDiagnoseIfAttrs(FD, ThisArg, Args, Loc);

// Diagnose variadic calls in SYCL.
if (FD && FD ->isVariadic() && getLangOpts().SYCLIsDevice &&
!isUnevaluatedContext() && !isKnownGoodSYCLDecl(FD))
SYCLDiagIfDeviceCode(Loc, diag::err_sycl_restrict)
<< Sema::KernelCallVariadicFunction;
}

/// CheckConstructorCall - Check a constructor call for correctness and safety
Expand Down
6 changes: 0 additions & 6 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5404,12 +5404,6 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, FunctionDecl *FDecl,

// Otherwise do argument promotion, (C99 6.5.2.2p7).
} else {
// Diagnose variadic calls in SYCL.
if (getLangOpts().SYCLIsDevice && !isUnevaluatedContext() &&
!isKnownGoodSYCLDecl(FDecl))
SYCLDiagIfDeviceCode(CallLoc, diag::err_sycl_restrict)
<< Sema::KernelCallVariadicFunction;

for (Expr *A : Args.slice(ArgIx)) {
ExprResult Arg = DefaultVariadicArgumentPromotion(A, CallType, FDecl);
Invalid |= Arg.isInvalid();
Expand Down
5 changes: 0 additions & 5 deletions clang/lib/Sema/SemaOverload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14230,11 +14230,6 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj,

// If this is a variadic call, handle args passed through "...".
if (Proto->isVariadic()) {
// Diagnose variadic calls in SYCL.
if (getLangOpts().SYCLIsDevice && !isUnevaluatedContext())
SYCLDiagIfDeviceCode(LParenLoc, diag::err_sycl_restrict)
<< Sema::KernelCallVariadicFunction;

// Promote the arguments (C99 6.5.2.2p7).
for (unsigned i = NumParams, e = Args.size(); i < e; i++) {
ExprResult Arg = DefaultVariadicArgumentPromotion(Args[i], VariadicMethod,
Expand Down
6 changes: 1 addition & 5 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,10 +1391,6 @@ static bool isKnownEmitted(Sema &S, FunctionDecl *FD) {
if (!FD)
return true; // Seen in LIT testing

// Templates are emitted when they're instantiated.
if (FD->isDependentContext())
return false;

if (FD->hasAttr<SYCLDeviceAttr>() || FD->hasAttr<SYCLKernelAttr>())
return true;

Expand All @@ -1409,7 +1405,7 @@ Sema::DeviceDiagBuilder Sema::SYCLDiagIfDeviceCode(SourceLocation Loc,
"Should only be called during SYCL compilation");
FunctionDecl *FD = dyn_cast<FunctionDecl>(getCurLexicalContext());
DeviceDiagBuilder::Kind DiagKind = [this, FD] {
if (ConstructingOpenCLKernel || (FD && FD->isDependentContext()))
if (ConstructingOpenCLKernel)
return DeviceDiagBuilder::K_Nop;
else if (isKnownEmitted(*this, FD))
return DeviceDiagBuilder::K_ImmediateWithCallStack;
Expand Down
5 changes: 5 additions & 0 deletions clang/test/SemaSYCL/inline-asm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ template <typename name, typename Func>
__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) {
// expected-note@+1 {{called by 'kernel_single_task<fake_kernel, (lambda}}
kernelFunc();
#ifdef LINUX_ASM
__asm__("int3"); // expected-error {{SYCL kernel cannot use inline assembly}}
#else
__asm int 3 // expected-error {{SYCL kernel cannot use inline assembly}}
#endif // LINUX_ASM
}

int main() {
Expand Down
18 changes: 18 additions & 0 deletions clang/test/SemaSYCL/sycl-callstack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// RUN: %clang_cc1 -fcxx-exceptions -fsycl-is-device -verify -fsyntax-only -std=c++17 %s

template <typename name, typename Func>
__attribute__((sycl_kernel))
void kernel_single_task(Func kernelFunc) {
// expected-note@+1 {{called by 'kernel_single_task}}
kernelFunc();
}

void foo() {
// expected-error@+1 {{SYCL kernel cannot use exceptions}}
throw 3;
}

int main() {
// expected-note@+1 {{called by 'operator()'}}
kernel_single_task<class fake_kernel>([]() { foo(); });
}
5 changes: 2 additions & 3 deletions clang/test/SemaSYCL/sycl-restrict.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ void eh_not_ok(void)

void usage(myFuncDef functionPtr) {

// expected-note@+1 {{called by 'usage'}}
eh_not_ok();

#if ALLOW_FP
Expand Down Expand Up @@ -170,6 +169,7 @@ int use2 ( a_type ab, a_type *abp ) {
return ns::glob +
// expected-error@+1 {{SYCL kernel cannot use a global variable}}
AnotherNS::moar_globals;
// expected-note@+1 {{called by 'use2'}}
eh_not_ok();
Check_RTTI_Restriction:: A *a;
Check_RTTI_Restriction:: isa_B(a);
Expand All @@ -180,16 +180,15 @@ int use2 ( a_type ab, a_type *abp ) {

template <typename name, typename Func>
__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) {
// expected-note@+1 {{called by 'kernel_single_task}}
kernelFunc();
a_type ab;
a_type *p;
// expected-note@+1 {{called by 'kernel_single_task'}}
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 really get why these notes are moving...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we are now diagnosing earlier. We aren't instantiating the template, we are doingil it during phase 1, so we don't know the call stack yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still is a bit unclear for me, but it's because I don't really know a lot about templates instantiating in clang, I suppose.
It seems like we previously added note above the kernelFunc() call, but now we are adding note (for same error, I suppose?) above the use2() call, because we only know call stack for path through use2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I believe that is because we're just hitting a different error 'first', and thus getting the dump from it. This file is a 'revert' of the changes I made to this in the last patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I understand that it's revert, but I'm afraid it's something wrong here too. So, doesn't that mean that we are still missing some error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of? Its more that the call-stack now is just different because we got there in a different way. We have a deferred diagnostic for that exception throw, and now the order that we're visiting the functions called by the kernel aren't the same. If you comment out the call to eh_not_ok in use2, it blames it on the call to useage. I think we're just doing a 'bredth-first' descent down the functions, so it is catching the 'shorter' path first (and thus taking up the 'delayed diagnostic'.).

Copy link
Contributor

Choose a reason for hiding this comment

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

Its more that the call-stack now is just different because we got there in a different way.

It's what I tried to understand :)

Just was a bit confused by "hitting a different error 'first'" phrase in your previous comment.

Now, let's consider the following example (sorry for typos, writing on the fly):

// let's have regular "sycl_kernel" function
template <typename name, typename Func>
void kernel_single_task(Func kernelFunc) {	
  kernelFunc();
}

// and template function
template<typename T> void some_foo(T t) {
  // Just do some bad things
 throw t; // or throw 3 if it's wrong, whatever
}

// And call our template function with bag things inside only from a kernel:
kernel_single_task<class fake_kernel>([]() { some_foo(3); });

Will this test case diagnosed through delayed diagnostics after the current patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this example would print the call stack kernel_single_task/operator()/some_foo. Since some_foo isn't a 'sycl_kernel' function (I'm imagining kernel_single_task has the attribute?), its diagnostic is delayed. The initial pass of kernel_single task doesn't know that it is going to be called with something that throws, so it doesn't diagnose.

That said, i tried your example, and it diagnoses 2x, so I want to figure out why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... it happens 2x because we're checking 'throw' without checking to see whether it is dependent. So we see it in both phases of the template. I don't know if openmp or CUDA have the same problem, or if they've worked around it. I'm guessing this: #1018 becomes more important to figure out now.

use2(ab, p);
}

int main() {
a_type ab;
// expected-note@+1 {{called by 'operator()'}}
kernel_single_task<class fake_kernel>([]() { usage( &addInt ); });
return 0;
}
Expand Down