From 4de5690538b0cce062a85621c46e05ec23ecd230 Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Wed, 8 Jan 2020 05:42:38 -0800 Subject: [PATCH 1/3] [SYCL] Move variadic call diagnostics to delayed diagnostics, fixes. One of our downstreams requires using the variadic call diagnostics from SYCL, however the SemaSYCL AST walker ends up being messy. This patch initially removes the check from the AST walker and instead implements its in normal SEMA via the delayed diagnostics mechanism. However, while evaluating this, we discovered that there are quite a few issues with how the diagnostics are emitted, so this patch includes quite a few cleanups to make sure the diagnostics are emitted properly. This results in some additional notes in some of the test. Additionally, the asm checks were previously implemented in BOTH delayed diagnostics as well as the AST walker, since the bugs in each aligned to have full coverage. This patch ends up removing it from the AST walking code since the other bugs were fixed. Signed-off-by: Erich Keane --- clang/include/clang/Sema/Sema.h | 6 ++++ clang/lib/Sema/SemaExpr.cpp | 6 ++++ clang/lib/Sema/SemaOverload.cpp | 5 +++ clang/lib/Sema/SemaSYCL.cpp | 39 ++++++++-------------- clang/test/SemaSYCL/inline-asm.cpp | 2 ++ clang/test/SemaSYCL/sycl-cconv.cpp | 4 ++- clang/test/SemaSYCL/sycl-restrict.cpp | 5 +-- clang/test/SemaSYCL/sycl-varargs-cconv.cpp | 2 +- clang/test/SemaSYCL/variadic-func-call.cpp | 39 ++++++++++++++++++++++ 9 files changed, 79 insertions(+), 29 deletions(-) create mode 100644 clang/test/SemaSYCL/variadic-func-call.cpp diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index e712745c963fb..46e2bb1f3d6a7 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -12015,6 +12015,11 @@ class Sema final { // is associated with. std::unique_ptr SyclIntHeader; + // Used to suppress diagnostics during kernel construction, since these were + // already emitted earlier. Diagnosing during Kernel emissions also skips the + // useful notes that shows where the kernel was called. + bool ConstructingOpenCLKernel = false; + public: void addSyclDeviceDecl(Decl *d) { SyclDeviceDecls.push_back(d); } SmallVectorImpl &syclDeviceDecls() { return SyclDeviceDecls; } @@ -12041,6 +12046,7 @@ class Sema final { KernelCallVariadicFunction }; DeviceDiagBuilder SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID); + bool isKnownGoodSYCLDecl(const Decl *D); void ConstructOpenCLKernel(FunctionDecl *KernelCallerFunc, MangleContext &MC); void MarkDevice(void); bool CheckSYCLCall(SourceLocation Loc, FunctionDecl *Callee); diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 1b563406552cd..695b94e1c913b 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -5380,6 +5380,12 @@ 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(); diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp index 1f77f2b9342cf..f416764650b48 100644 --- a/clang/lib/Sema/SemaOverload.cpp +++ b/clang/lib/Sema/SemaOverload.cpp @@ -14027,6 +14027,11 @@ 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, diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 23ef6c4ad920c..19fb2ee3e18af 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -184,7 +184,7 @@ static bool IsSyclMathFunc(unsigned BuiltinID) { return true; } -static bool isKnownGoodDecl(const Decl *D) { +bool Sema::isKnownGoodSYCLDecl(const Decl *D) { if (const FunctionDecl *FD = dyn_cast(D)) { const IdentifierInfo *II = FD->getIdentifier(); const DeclContext *DC = FD->getDeclContext(); @@ -326,7 +326,7 @@ class MarkDeviceFunction : public RecursiveASTVisitor { bool VisitDeclRefExpr(DeclRefExpr *E) { Decl* D = E->getDecl(); - if (isKnownGoodDecl(D)) + if (SemaRef.isKnownGoodSYCLDecl(D)) return true; CheckSYCLType(E->getType(), E->getSourceRange()); @@ -372,18 +372,6 @@ class MarkDeviceFunction : public RecursiveASTVisitor { return true; } - bool VisitGCCAsmStmt(GCCAsmStmt *S) { - SemaRef.Diag(S->getBeginLoc(), diag::err_sycl_restrict) - << Sema::KernelUseAssembly; - return true; - } - - bool VisitMSAsmStmt(MSAsmStmt *S) { - SemaRef.Diag(S->getBeginLoc(), diag::err_sycl_restrict) - << Sema::KernelUseAssembly; - return true; - } - // The call graph for this translation unit. CallGraph SYCLCG; // The set of functions called by a kernel function. @@ -549,9 +537,6 @@ class MarkDeviceFunction : public RecursiveASTVisitor { } } } else if (const auto *FPTy = dyn_cast(Ty)) { - if (FPTy->isVariadic() && SemaRef.getLangOpts().SYCLIsDevice) - SemaRef.SYCLDiagIfDeviceCode(Loc.getBegin(), diag::err_sycl_restrict) - << Sema::KernelCallVariadicFunction; for (const auto &ParamTy : FPTy->param_types()) if (!CheckSYCLType(ParamTy, Loc, Visited)) return false; @@ -1308,8 +1293,10 @@ void Sema::ConstructOpenCLKernel(FunctionDecl *KernelCallerFunc, // in different translation units. OpenCLKernel->setImplicitlyInline(KernelCallerFunc->isInlined()); + ConstructingOpenCLKernel = true; CompoundStmt *OpenCLKernelBody = CreateOpenCLKernelBody(*this, KernelCallerFunc, OpenCLKernel); + ConstructingOpenCLKernel = false; OpenCLKernel->setBody(OpenCLKernelBody); addSyclDeviceDecl(OpenCLKernel); } @@ -1404,13 +1391,13 @@ static bool isKnownEmitted(Sema &S, FunctionDecl *FD) { if (!FD) return true; // Seen in LIT testing - if (FD->hasAttr() || FD->hasAttr()) - return true; - // Templates are emitted when they're instantiated. if (FD->isDependentContext()) return false; + if (FD->hasAttr() || FD->hasAttr()) + return true; + // Otherwise, the function is known-emitted if it's in our set of // known-emitted functions. return S.DeviceKnownEmittedFns.count(FD) > 0; @@ -1420,18 +1407,20 @@ Sema::DeviceDiagBuilder Sema::SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID) { assert(getLangOpts().SYCLIsDevice && "Should only be called during SYCL compilation"); - DeviceDiagBuilder::Kind DiagKind = [this] { - if (isKnownEmitted(*this, dyn_cast(CurContext))) + FunctionDecl *FD = dyn_cast(CurContext); + DeviceDiagBuilder::Kind DiagKind = [this, FD] { + if (ConstructingOpenCLKernel || (FD && FD->isDependentContext())) + return DeviceDiagBuilder::K_Nop; + else if (isKnownEmitted(*this, FD)) return DeviceDiagBuilder::K_ImmediateWithCallStack; return DeviceDiagBuilder::K_Deferred; }(); - return DeviceDiagBuilder(DiagKind, Loc, DiagID, - dyn_cast(CurContext), *this); + return DeviceDiagBuilder(DiagKind, Loc, DiagID, FD, *this); } bool Sema::CheckSYCLCall(SourceLocation Loc, FunctionDecl *Callee) { assert(Callee && "Callee may not be null."); - FunctionDecl *Caller = getCurFunctionDecl(); + FunctionDecl *Caller = dyn_cast(getCurLexicalContext()); // If the caller is known-emitted, mark the callee as known-emitted. // Otherwise, mark the call in our call graph so we can traverse it later. diff --git a/clang/test/SemaSYCL/inline-asm.cpp b/clang/test/SemaSYCL/inline-asm.cpp index cab5ff20b2659..6290c01367bfb 100644 --- a/clang/test/SemaSYCL/inline-asm.cpp +++ b/clang/test/SemaSYCL/inline-asm.cpp @@ -21,11 +21,13 @@ void bar() { template __attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) { + // expected-note@+1 {{called by 'kernel_single_task([]() { bar(); }); return 0; } diff --git a/clang/test/SemaSYCL/sycl-cconv.cpp b/clang/test/SemaSYCL/sycl-cconv.cpp index a442974023377..5aea284e814a4 100644 --- a/clang/test/SemaSYCL/sycl-cconv.cpp +++ b/clang/test/SemaSYCL/sycl-cconv.cpp @@ -8,7 +8,7 @@ __inline __cdecl __attribute__((sycl_device)) int foo() { return 0; } __inline __cdecl int moo() { return 0; } void bar() { - printf("hello\n"); // expected-no-error + //printf("hello\n"); // expected-no-error } template @@ -18,10 +18,12 @@ __cdecl __attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) { printf("cannot call from here\n"); // expected-no-error@+1 moo(); + // expected-note@+1{{called by}} kernelFunc(); } int main() { + //expected-note@+2 {{in instantiation of}} //expected-error@+1 {{SYCL kernel cannot call a variadic function}} kernel_single_task([]() { printf("world\n"); }); bar(); diff --git a/clang/test/SemaSYCL/sycl-restrict.cpp b/clang/test/SemaSYCL/sycl-restrict.cpp index 083b7f775f2cc..89dd0adeffe01 100644 --- a/clang/test/SemaSYCL/sycl-restrict.cpp +++ b/clang/test/SemaSYCL/sycl-restrict.cpp @@ -124,6 +124,7 @@ void eh_not_ok(void) void usage(myFuncDef functionPtr) { + // expected-note@+1 {{called by 'usage'}} eh_not_ok(); #if ALLOW_FP @@ -169,7 +170,6 @@ 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); @@ -180,15 +180,16 @@ int use2 ( a_type ab, a_type *abp ) { template __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'}} use2(ab, p); } int main() { a_type ab; + // expected-note@+1 {{called by 'operator()'}} kernel_single_task([]() { usage( &addInt ); }); return 0; } diff --git a/clang/test/SemaSYCL/sycl-varargs-cconv.cpp b/clang/test/SemaSYCL/sycl-varargs-cconv.cpp index 6ba89aa9af9de..35140b0e6967c 100644 --- a/clang/test/SemaSYCL/sycl-varargs-cconv.cpp +++ b/clang/test/SemaSYCL/sycl-varargs-cconv.cpp @@ -39,7 +39,7 @@ void bar() { template __attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) { - kernelFunc(); + kernelFunc(); //expected-note 2+ {{called by 'kernel_single_task}} } int main() { diff --git a/clang/test/SemaSYCL/variadic-func-call.cpp b/clang/test/SemaSYCL/variadic-func-call.cpp new file mode 100644 index 0000000000000..515c0ce68991e --- /dev/null +++ b/clang/test/SemaSYCL/variadic-func-call.cpp @@ -0,0 +1,39 @@ +// RUN: %clang_cc1 -triple spir64-unknown-unknown -fsycl-is-device -fsyntax-only -verify %s + +void variadic(int, ...); +namespace NS { +void variadic(int, ...); +} + +struct S { + S(int, ...); + void operator()(int, ...); +}; + +void foo() { + auto x = [](int, ...) {}; + x(5, 10); //expected-error{{SYCL kernel cannot call a variadic function}} +} + +void overloaded(int, int); +void overloaded(int, ...); +template +__attribute__((sycl_kernel)) void task(Func KF) { + KF(); // expected-note 2 {{called by 'task}} +} + +int main() { + task([]() { + variadic(5); //expected-error{{SYCL kernel cannot call a variadic function}} + variadic(5, 2); //expected-error{{SYCL kernel cannot call a variadic function}} + NS::variadic(5, 3); //expected-error{{SYCL kernel cannot call a variadic function}} + S s(5, 4); //expected-error{{SYCL kernel cannot call a variadic function}} + S s2(5); //expected-error{{SYCL kernel cannot call a variadic function}} + s(5, 5); //expected-error{{SYCL kernel cannot call a variadic function}} + s2(5); //expected-error{{SYCL kernel cannot call a variadic function}} + foo(); //expected-note{{called by 'operator()'}} + overloaded(5, 6); //expected-no-error + overloaded(5, s); //expected-error{{SYCL kernel cannot call a variadic function}} + overloaded(5); //expected-error{{SYCL kernel cannot call a variadic function}} + }); +} From 626431c6952c4934ef733c1fb45c7f679e670117 Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Fri, 10 Jan 2020 10:21:30 -0800 Subject: [PATCH 2/3] [SYCL] Revert inadvertant change in test from previous commit @premanandrao saw that I had accidentially left a line in the test commented out while debugging. Revert the commented out line to still be testing what it is supposed to be testing. Signed-off-by: Erich Keane --- clang/lib/Sema/SemaSYCL.cpp | 2 +- clang/test/SemaSYCL/sycl-cconv.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 19fb2ee3e18af..477ec98b4d48f 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -1407,7 +1407,7 @@ Sema::DeviceDiagBuilder Sema::SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID) { assert(getLangOpts().SYCLIsDevice && "Should only be called during SYCL compilation"); - FunctionDecl *FD = dyn_cast(CurContext); + FunctionDecl *FD = dyn_cast(getCurLexicalContext()); DeviceDiagBuilder::Kind DiagKind = [this, FD] { if (ConstructingOpenCLKernel || (FD && FD->isDependentContext())) return DeviceDiagBuilder::K_Nop; diff --git a/clang/test/SemaSYCL/sycl-cconv.cpp b/clang/test/SemaSYCL/sycl-cconv.cpp index 5aea284e814a4..a189b57730a50 100644 --- a/clang/test/SemaSYCL/sycl-cconv.cpp +++ b/clang/test/SemaSYCL/sycl-cconv.cpp @@ -8,7 +8,7 @@ __inline __cdecl __attribute__((sycl_device)) int foo() { return 0; } __inline __cdecl int moo() { return 0; } void bar() { - //printf("hello\n"); // expected-no-error + printf("hello\n"); // expected-no-error } template From 04bfe60573aacbc3de2fb50836566e6c1fd2f332 Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Tue, 14 Jan 2020 09:37:22 -0800 Subject: [PATCH 3/3] [SYCL] Remove FIXME comment that this invalidates. As brought up by @bader in review. Signed-off-by: Erich Keane --- clang/lib/Sema/SemaType.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp index 81762077d56ac..732b3e06e8ccd 100644 --- a/clang/lib/Sema/SemaType.cpp +++ b/clang/lib/Sema/SemaType.cpp @@ -4685,7 +4685,6 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state, // OpenCL doesn't support variadic functions and blocks // (s6.9.e and s6.12.5 OpenCL v2.0) except for printf. // We also allow here any toolchain reserved identifiers. - // FIXME: Use deferred diagnostics engine to skip host side issues. if (FTI.isVariadic && !LangOpts.SYCLIsDevice && !(D.getIdentifier() &&