Misc cleanup.

- Added/edited lots of comments.
- Expanded testing for sycl_kernel_launch lookup and reorganized the tests
  to make it easier to identify and/or plug testing gaps.
- Added a missing sycl_kernel_entry_point attribute in a test.
- Reordered various declarations for better grouping and consistency.
- Renamed some variables and functions.
- Removed an unnecessary include directive.
- Adjusted the location used for the implicit sycl_kernel_launch call. The
  opening brace of the original function body is used where available and
  the general function location is used otherwise.
- Corrected lookup to unconditionally look for a template name; previously
  a spurious error about a 'template' keyword could be issued.
- Added missing code gen checks for use of a member function as the SYCL
  kernel entry point function.
- Other misc style edits for consistency.

Author: tahonermann

clang/include/clang/AST/StmtSYCL.h

@@ -100,23 +100,30 @@ class SYCLKernelCallStmt : public Stmt {
 };
 
 // UnresolvedSYCLKernelCallStmt represents an invocation of a SYCL kernel in
-// a dependent context for which lookup of the sycl_enqueue_kernel_launch
-// identifier cannot be performed. These statements are transformed to
-// SYCLKernelCallStmt during template instantiation.
+// a dependent context for which lookup of the sycl_kernel_launch identifier
+// cannot be performed. These statements are transformed to SYCLKernelCallStmt
+// during template instantiation.
 class UnresolvedSYCLKernelCallStmt : public Stmt {
   friend class ASTStmtReader;
+  friend class ASTStmtWriter;
+
+private:
   Stmt *OriginalStmt = nullptr;
   // KernelLaunchIdExpr stores an UnresolvedLookupExpr or UnresolvedMemberExpr
   // corresponding to the SYCL kernel launch function for which a call
   // will be synthesized during template instantiation.
   Expr *KernelLaunchIdExpr = nullptr;
+
   UnresolvedSYCLKernelCallStmt(CompoundStmt *CS, Expr *IdExpr)
       : Stmt(UnresolvedSYCLKernelCallStmtClass), OriginalStmt(CS),
         KernelLaunchIdExpr(IdExpr) {}
 
-  void setKernelLaunchIdExpr(Expr *IdExpr) { KernelLaunchIdExpr = IdExpr; }
+  /// Set the original statement.
   void setOriginalStmt(CompoundStmt *CS) { OriginalStmt = CS; }
 
+  /// Set the kernel launch ID expression.
+  void setKernelLaunchIdExpr(Expr *IdExpr) { KernelLaunchIdExpr = IdExpr; }
+
 public:
   static UnresolvedSYCLKernelCallStmt *
   Create(const ASTContext &C, CompoundStmt *CS, Expr *IdExpr) {
@@ -127,12 +134,16 @@ class UnresolvedSYCLKernelCallStmt : public Stmt {
     return new (C) UnresolvedSYCLKernelCallStmt(nullptr, nullptr);
   }
 
-  Expr *getKernelLaunchIdExpr() const { return KernelLaunchIdExpr; }
+  /// Retrieve the original statement.
   CompoundStmt *getOriginalStmt() { return cast<CompoundStmt>(OriginalStmt); }
   const CompoundStmt *getOriginalStmt() const {
     return cast<CompoundStmt>(OriginalStmt);
   }
 
+  /// Retrieve the kernel launch ID expression.
+  Expr *getKernelLaunchIdExpr() { return KernelLaunchIdExpr; }
+  const Expr *getKernelLaunchIdExpr() const { return KernelLaunchIdExpr; }
+
   SourceLocation getBeginLoc() const LLVM_READONLY {
     return getOriginalStmt()->getBeginLoc();
   }

clang/include/clang/Sema/SemaSYCL.h

@@ -66,11 +66,32 @@ class SemaSYCL : public SemaBase {
 
   void CheckSYCLExternalFunctionDecl(FunctionDecl *FD);
   void CheckSYCLEntryPointFunctionDecl(FunctionDecl *FD);
+
+  /// Builds an expression for the lookup of a 'sycl_kernel_launch' template
+  /// with 'KernelName' as an explicit template argument. Lookup is performed
+  /// as if from the first statement of the body of 'FD' and thus requires
+  /// searching the scopes that exist at parse time. This function therefore
+  /// requires the current semantic context to be the definition of 'FD'. In a
+  /// dependent context, the returned expression will be an UnresolvedLookupExpr
+  /// or an UnresolvedMemberExpr. In a non-dependent context, the returned
+  /// expression will be a DeclRefExpr or MemberExpr. If lookup fails, a null
+  /// error result is returned. The resulting expression is intended to be
+  /// passed as the 'LaunchIdExpr' argument in a call to either
+  /// BuildSYCLKernelCallStmt() or BuildUnresolvedSYCLKernelCallStmt() after
+  /// the function body has been parsed.
+  ExprResult BuildSYCLKernelLaunchIdExpr(FunctionDecl *FD, QualType KernelName);
+
+  /// Builds a SYCLKernelCallStmt to wrap 'Body' and to be used as the body of
+  /// 'FD'. 'LaunchIdExpr' specifies the lookup result returned by a previous
+  /// call to BuildSYCLKernelLaunchIdExpr().
   StmtResult BuildSYCLKernelCallStmt(FunctionDecl *FD, CompoundStmt *Body,
                                      Expr *LaunchIdExpr);
-  ExprResult BuildSYCLKernelLaunchIdExpr(FunctionDecl *FD, QualType KNT);
-  StmtResult BuildUnresolvedSYCLKernelCallStmt(CompoundStmt *CS,
-                                                     Expr *IdExpr);
+
+  /// Builds an UnresolvedSYCLKernelCallStmt to wrap 'Body'. 'LaunchIdExpr'
+  /// specifies the lookup result returned by a previous call to
+  /// BuildSYCLKernelLaunchIdExpr().
+  StmtResult BuildUnresolvedSYCLKernelCallStmt(CompoundStmt *Body,
+                                               Expr *LaunchIdExpr);
 };
 
 } // namespace clang

clang/lib/AST/ComputeDependence.cpp

@@ -16,7 +16,6 @@
 #include "clang/AST/ExprConcepts.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
-#include "clang/AST/StmtSYCL.h"
 #include "clang/Basic/ExceptionSpecificationType.h"
 #include "llvm/ADT/ArrayRef.h"
 

clang/lib/CodeGen/CGStmt.cpp

@@ -104,6 +104,7 @@ void CodeGenFunction::EmitStmt(const Stmt *S, ArrayRef<const Attr *> Attrs) {
   case Stmt::SEHExceptStmtClass:
   case Stmt::SEHFinallyStmtClass:
   case Stmt::MSDependentExistsStmtClass:
+  case Stmt::UnresolvedSYCLKernelCallStmtClass:
     llvm_unreachable("invalid statement class to emit generically");
   case Stmt::NullStmtClass:
   case Stmt::CompoundStmtClass:
@@ -117,7 +118,6 @@ void CodeGenFunction::EmitStmt(const Stmt *S, ArrayRef<const Attr *> Attrs) {
   case Stmt::CaseStmtClass:
   case Stmt::SEHLeaveStmtClass:
   case Stmt::SYCLKernelCallStmtClass:
-  case Stmt::UnresolvedSYCLKernelCallStmtClass:
     llvm_unreachable("should have emitted these statements as simple");
 
 #define STMT(Type, Base)

clang/lib/Sema/SemaDecl.cpp

@@ -16228,10 +16228,15 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
 
   if (FD && !FD->isInvalidDecl() &&
       FD->hasAttr<SYCLKernelEntryPointAttr>() && FnBodyScope) {
-    // Building KernelLaunchIdExpr requires performing an unqualified lookup
-    // which can only be done correctly while the stack of parsing scopes is
-    // alive, so we do it here when we start parsing function body even if it is
-    // a templated function.
+    // An implicit call expression is synthesized for functions declared with
+    // the sycl_kernel_entry_point attribute. The call may resolve to a
+    // function template, a member function template, or a call operator
+    // of a variable template depending on the results of unqualified lookup
+    // for 'sycl_kernel_launch' from the beginning of the function body.
+    // Performing that lookup requires the stack of parsing scopes active
+    // when the definition is parsed and is thus done here; the result is
+    // cached in FunctionScopeInfo and used to synthesize the (possibly
+    // unresolved) call expression after the function body has been parsed.
     const auto *SKEPAttr = FD->getAttr<SYCLKernelEntryPointAttr>();
     if (!SKEPAttr->isInvalidAttr()) {
       ExprResult LaunchIdExpr =
@@ -16441,8 +16446,11 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, bool IsInstantiation,
       SKEPAttr->setInvalidAttr();
     }
 
-    // We don't need to build SYCLKernelCallStmt for template instantiations
-    // since it was already created by template instantiator.
+    // Build an unresolved SYCL kernel call statement for a function template,
+    // validate that a SYCL kernel call statement was instantiated for an
+    // (implicit or explicit) instantiation of a function template, or otherwise
+    // build a (resolved) SYCL kernel call statement for a non-templated
+    // function or an explicit specialization.
     if (Body && !SKEPAttr->isInvalidAttr()) {
       StmtResult SR;
       if (FD->isTemplated()) {

clang/lib/Sema/SemaSYCL.cpp

@@ -392,85 +392,82 @@ void SemaSYCL::CheckSYCLEntryPointFunctionDecl(FunctionDecl *FD) {
 
 ExprResult SemaSYCL::BuildSYCLKernelLaunchIdExpr(FunctionDecl *FD,
                                                  QualType KNT) {
+  // The current context must be the function definition context to ensure
+  // that name lookup is performed within the correct scope.
+  assert(SemaRef.CurContext == FD);
 
-  ASTContext &Ctx = SemaRef.getASTContext();
-  // Some routines need a valid source location to work correctly.
-  SourceLocation BodyLoc =
-      FD->getEndLoc().isValid() ? FD->getEndLoc() : FD->getLocation();
+  // An appropriate source location is required to emit diagnostics if
+  // lookup fails to produce an overload set. The desired location is the
+  // start of the function body, but that is not yet available since the
+  // body of the function has not yet been set when this function is called.
+  // The general location of the function is used instead.
+  SourceLocation Loc = FD->getLocation();
 
-  IdentifierInfo &LaunchFooName =
+  ASTContext &Ctx = SemaRef.getASTContext();
+  IdentifierInfo &SYCLKernelLaunchID =
       Ctx.Idents.get("sycl_kernel_launch", tok::TokenKind::identifier);
 
-  // Perform overload resolution for a call to an accessible (member) function
-  // template named 'sycl_kernel_launch' "from within the definition of
-  // FD where":
-  // - The kernel name type is passed as the first template argument.
-  // - Any remaining template parameters are deduced from the function
-  //   arguments or assigned by default template arguments.
-  // - 'this' is passed as the implicit function argument if 'FD' is a
-  //   non-static member function.
-  // - The name of the kernel, expressed as a string literal, is passed as the
-  //   first function argument.
-  // - The parameters of FD are forwarded as-if by 'std::forward()' as the
-  //   remaining explicit function arguments.
-  // - Any remaining function arguments are initialized by default arguments.
-  LookupResult Result(SemaRef, &LaunchFooName, BodyLoc,
+  // Perform ordinary name lookup for a function or variable template that
+  // accepts a single type template argument.
+  LookupResult Result(SemaRef, &SYCLKernelLaunchID, Loc,
                       Sema::LookupOrdinaryName);
-  CXXScopeSpec SS;
-  SemaRef.LookupTemplateName(Result, SemaRef.getCurScope(), SS,
-                             /*ObjectType=*/QualType(),
-                             /*EnteringContext=*/false, BodyLoc);
+  CXXScopeSpec EmptySS;
+  SemaRef.LookupTemplateName(Result, SemaRef.getCurScope(), EmptySS,
+                             /*ObjectType*/ QualType(),
+                             /*EnteringContext*/ false,
+                             Sema::TemplateNameIsRequired);
 
   if (Result.empty() || Result.isAmbiguous()) {
-    SemaRef.Diag(BodyLoc, SemaRef.getLangOpts().SYCLIsHost
-                              ? diag::err_sycl_host_no_launch_function
-                              : diag::warn_sycl_device_no_host_launch_function);
-    SemaRef.Diag(BodyLoc, diag::note_sycl_host_launch_function);
+    SemaRef.Diag(Loc, SemaRef.getLangOpts().SYCLIsHost
+                          ? diag::err_sycl_host_no_launch_function
+                          : diag::warn_sycl_device_no_host_launch_function);
+    SemaRef.Diag(Loc, diag::note_sycl_host_launch_function);
 
     return ExprError();
   }
 
-  TemplateArgumentListInfo TALI{BodyLoc, BodyLoc};
+  TemplateArgumentListInfo TALI{Loc, Loc};
   TemplateArgument KNTA = TemplateArgument(KNT);
   TemplateArgumentLoc TAL =
-      SemaRef.getTrivialTemplateArgumentLoc(KNTA, QualType(), BodyLoc);
+      SemaRef.getTrivialTemplateArgumentLoc(KNTA, QualType(), Loc);
   TALI.addArgument(TAL);
   ExprResult IdExpr;
-  if (SemaRef.isPotentialImplicitMemberAccess(SS, Result,
-                                              /*IsAddressOfOperand=*/false))
-    // BuildPossibleImplicitMemberExpr creates UnresolvedMemberExpr. Using it
-    // allows to pass implicit/explicit this argument automatically.
-    IdExpr = SemaRef.BuildPossibleImplicitMemberExpr(SS, BodyLoc, Result, &TALI,
+  if (SemaRef.isPotentialImplicitMemberAccess(EmptySS, Result,
+                                              /*IsAddressOfOperand*/ false))
+    // The lookup result allows for a possible implicit member access that
+    // would require an implicit or explicit 'this' argument.
+    IdExpr = SemaRef.BuildPossibleImplicitMemberExpr(EmptySS, SourceLocation(),
+                                                     Result, &TALI,
                                                      SemaRef.getCurScope());
   else
-    IdExpr = SemaRef.BuildTemplateIdExpr(SS, BodyLoc, Result,
-                                         /*RequiresADL=*/true, &TALI);
+    IdExpr = SemaRef.BuildTemplateIdExpr(EmptySS, SourceLocation(), Result,
+                                         /*RequiresADL*/ true, &TALI);
 
-  // Can happen if SKEP attributed function is a static member, but the launcher
-  // is a regular member. Perhaps emit a note saying that we're in host code
-  // synthesis.
+  // The resulting expression may be invalid if, for example, 'FD' is a
+  // non-static member function and sycl_kernel_launch lookup selects a
+  // member function (which would require a 'this' argument which is
+  // not available).
   if (IdExpr.isInvalid())
     return ExprError();
 
   return IdExpr;
 }
 
-StmtResult SemaSYCL::BuildUnresolvedSYCLKernelCallStmt(CompoundStmt *CS,
-                                                             Expr *IdExpr) {
-  return UnresolvedSYCLKernelCallStmt::Create(SemaRef.getASTContext(), CS,
-                                                    IdExpr);
-}
-
 namespace {
 
-void PrepareKernelArgumentsForKernelLaunch(SmallVectorImpl<Expr *> &Args,
-                                                  const SYCLKernelInfo *SKI,
-                                                  Sema &SemaRef,
-                                                  SourceLocation Loc) {
-  assert(SKI && "Need a kernel!");
-  ASTContext &Ctx = SemaRef.getASTContext();
+// Constructs the arguments to be passed for the SYCL kernel launch call.
+// The first argument is a string literal that contains the SYCL kernel
+// name. The remaining arguments are the parameters of 'FD'.
+void BuildSYCLKernelLaunchCallArgs(Sema &SemaRef, FunctionDecl *FD,
+                                   const SYCLKernelInfo *SKI,
+                                   SmallVectorImpl<Expr *> &Args,
+                                   SourceLocation Loc) {
+  // The current context must be the function definition context to ensure
+  // that parameter references occur within the correct scope.
+  assert(SemaRef.CurContext == FD);
 
   // Prepare a string literal that contains the kernel name.
+  ASTContext &Ctx = SemaRef.getASTContext();
   const std::string KernelName = SKI->GetKernelName();
   QualType KernelNameCharTy = Ctx.CharTy.withConst();
   llvm::APInt KernelNameSize(Ctx.getTypeSize(Ctx.getSizeType()),
@@ -482,30 +479,23 @@ void PrepareKernelArgumentsForKernelLaunch(SmallVectorImpl<Expr *> &Args,
                             /*Pascal*/ false, KernelNameArrayTy, Loc);
   Args.push_back(KernelNameExpr);
 
-  // Right now we simply forward the arguments of the skep-attributed function.
-  // With decomposition present there can be another logic.
-  // Make sure to use CurContext to avoid diagnostics that we're using a
-  // variable coming from another context. The function should be the same as in
-  // the kernel info though.
-  auto *FD = cast<FunctionDecl>(SemaRef.CurContext);
-  assert(declaresSameEntity(FD, SKI->getKernelEntryPointDecl()));
   for (ParmVarDecl *PVD : FD->parameters()) {
     QualType ParamType = PVD->getOriginalType().getNonReferenceType();
     Expr *DRE = SemaRef.BuildDeclRefExpr(PVD, ParamType, VK_LValue, Loc);
-    assert(DRE);
     Args.push_back(DRE);
   }
 }
 
-StmtResult BuildSYCLKernelLaunchStmt(Sema &SemaRef,
-                                            const SYCLKernelInfo *SKI,
-                                            Expr *IdExpr, SourceLocation Loc) {
+// Constructs the SYCL kernel launch call.
+StmtResult BuildSYCLKernelLaunchCallStmt(Sema &SemaRef, FunctionDecl *FD,
+                                         const SYCLKernelInfo *SKI,
+                                         Expr *IdExpr, SourceLocation Loc) {
   SmallVector<Stmt *> Stmts;
-  assert(SKI && "Need a Kernel!");
 
+  // IdExpr may be null if name lookup failed.
   if (IdExpr) {
     llvm::SmallVector<Expr *, 12> Args;
-    PrepareKernelArgumentsForKernelLaunch(Args, SKI, SemaRef, Loc);
+    BuildSYCLKernelLaunchCallArgs(SemaRef, FD, SKI, Args, Loc);
     ExprResult LaunchResult =
         SemaRef.BuildCallExpr(SemaRef.getCurScope(), IdExpr, Loc, Args, Loc);
     if (LaunchResult.isInvalid())
@@ -630,14 +620,20 @@ StmtResult SemaSYCL::BuildSYCLKernelCallStmt(FunctionDecl *FD,
       BuildSYCLKernelEntryPointOutline(SemaRef, FD, Body);
   assert(OFD);
 
-  // Build host kernel launch stmt.
-  SourceLocation BodyLoc =
-      FD->getEndLoc().isValid() ? FD->getEndLoc() : FD->getLocation();
-  StmtResult LaunchRes =
-      BuildSYCLKernelLaunchStmt(SemaRef, &SKI, LaunchIdExpr, BodyLoc);
+  // Build the host kernel launch statement. An appropriate source location
+  // is required to emit diagnostics.
+  SourceLocation Loc = Body->getLBracLoc();
+  StmtResult LaunchResult =
+      BuildSYCLKernelLaunchCallStmt(SemaRef, FD, &SKI, LaunchIdExpr, Loc);
 
   Stmt *NewBody =
-      new (getASTContext()) SYCLKernelCallStmt(Body, LaunchRes.get(), OFD);
+      new (getASTContext()) SYCLKernelCallStmt(Body, LaunchResult.get(), OFD);
 
   return NewBody;
 }
+
+StmtResult SemaSYCL::BuildUnresolvedSYCLKernelCallStmt(CompoundStmt *Body,
+                                                       Expr *LaunchIdExpr) {
+  return UnresolvedSYCLKernelCallStmt::Create(SemaRef.getASTContext(), Body,
+                                              LaunchIdExpr);
+}

clang/lib/Sema/TreeTransform.h

@@ -12975,7 +12975,6 @@ StmtResult TreeTransform<Derived>::TransformUnresolvedSYCLKernelCallStmt(
     return StmtError();
 
   ExprResult IdExpr = getDerived().TransformExpr(S->getKernelLaunchIdExpr());
-
   if (IdExpr.isInvalid())
      return StmtError();
 

clang/test/ASTSYCL/ast-dump-sycl-kernel-call-stmt.cpp

@@ -34,7 +34,7 @@ template<int> struct K {
   void operator()(Ts...) const {}
 };
 
-template <typename KernelName, typename... Ts>
+template<typename KernelName, typename... Ts>
 void sycl_kernel_launch(const char *, Ts...) {}
 
 [[clang::sycl_kernel_entry_point(KN<1>)]]
@@ -373,18 +373,18 @@ void skep8(S8 k) {
 // CHECK:      | `-SYCLKernelEntryPointAttr {{.*}}
 
 class Handler {
-template <typename KernelName, typename... Ts>
-void sycl_kernel_launch(const char *, Ts...) {}
+  template <typename KNT, typename... Ts>
+  void sycl_kernel_launch(const char *, Ts...) {}
 public:
-template<typename KNT, typename KT>
-[[clang::sycl_kernel_entry_point(KNT)]]
-void skep9(KT k, int a, int b) {
-  k(a, b);
-}
+  template<typename KNT, typename KT>
+  [[clang::sycl_kernel_entry_point(KNT)]]
+  void skep9(KT k, int a, int b) {
+    k(a, b);
+  }
 };
 void foo() {
   Handler H;
-  H.skep9<KN<9>>([=](int a, int b){return a+b;}, 1, 2);
+  H.skep9<KN<9>>([=] (int a, int b) { return a+b; }, 1, 2);
 }
 
 // CHECK: | |-FunctionTemplateDecl {{.*}} skep9

clang/test/ASTSYCL/ast-dump-sycl-kernel-entry-point.cpp

@@ -28,8 +28,8 @@
 // A unique kernel name type is required for each declared kernel entry point.
 template<int, int=0> struct KN;
 
-template <typename KernelName, typename... Tys>
-void sycl_kernel_launch(const char *, Tys &&...Args) {}
+template<typename KernelName, typename... Ts>
+void sycl_kernel_launch(const char *, Ts... Args) {}
 
 [[clang::sycl_kernel_entry_point(KN<1>)]]
 void skep1() {