[SYCL] Move variadic call diagnostics to delayed diagnostics, fixes. (#998)

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.

Remove FIXME comment that this invalidates.

Signed-off-by: Erich Keane <[email protected]>

Author: Erich Keane

clang/include/clang/Sema/Sema.h

@@ -12049,6 +12049,11 @@ class Sema final {
   // is associated with.
   std::unique_ptr<SYCLIntegrationHeader> 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<Decl *> &syclDeviceDecls() { return SyclDeviceDecls; }
@@ -12075,6 +12080,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);

clang/lib/Sema/SemaExpr.cpp

@@ -5404,6 +5404,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();

clang/lib/Sema/SemaOverload.cpp

@@ -14230,6 +14230,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,

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<FunctionDecl>(D)) {
     const IdentifierInfo *II = FD->getIdentifier();
     const DeclContext *DC = FD->getDeclContext();
@@ -326,7 +326,7 @@ class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
 
   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<MarkDeviceFunction> {
     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<MarkDeviceFunction> {
         }
       }
     } else if (const auto *FPTy = dyn_cast<FunctionProtoType>(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<SYCLDeviceAttr>() || FD->hasAttr<SYCLKernelAttr>())
-    return true;
-
   // Templates are emitted when they're instantiated.
   if (FD->isDependentContext())
     return false;
 
+  if (FD->hasAttr<SYCLDeviceAttr>() || FD->hasAttr<SYCLKernelAttr>())
+    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<FunctionDecl>(CurContext)))
+  FunctionDecl *FD = dyn_cast<FunctionDecl>(getCurLexicalContext());
+  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<FunctionDecl>(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<FunctionDecl>(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.

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() &&

clang/test/SemaSYCL/inline-asm.cpp

@@ -21,11 +21,13 @@ void bar() {
 
 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();
 }
 
 int main() {
   foo();
+  // expected-note@+1 {{called by 'operator()'}}
   kernel_single_task<class fake_kernel>([]() { bar(); });
   return 0;
 }

clang/test/SemaSYCL/sycl-cconv.cpp

@@ -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<class fake_kernel>([]() { printf("world\n"); });
   bar();

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 <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'}}
   use2(ab, p);
 }
 
 int main() {
   a_type ab;
+  // expected-note@+1 {{called by 'operator()'}}
   kernel_single_task<class fake_kernel>([]() { usage(  &addInt ); });
   return 0;
 }

clang/test/SemaSYCL/sycl-varargs-cconv.cpp

@@ -39,7 +39,7 @@ void bar() {
 
 template <typename name, typename Func>
 __attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) {
-  kernelFunc();
+  kernelFunc(); //expected-note 2+ {{called by 'kernel_single_task}}
 }
 
 int main() {

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 <typename, typename Func>
+__attribute__((sycl_kernel)) void task(Func KF) {
+  KF(); // expected-note 2 {{called by 'task}}
+}
+
+int main() {
+  task<class FK>([]() {
+    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}}
+  });
+}