[SYCL] Change unique-stable-name to just use the lambda's mangled name. (#4457)

Currently, whether a lambda is used in a kernel is used to determine the
result of `__builtin_sycl_unique_stable_name`.This leads to a sitaution
where using a lambda in a kernel that is lexically higher than an
existing kernel will change the result of the builtin, causing a build
failure.

This patch replaces this with JUST the lambda's itanium-mangled name.
This has the limitation in that it is not stable/unique in light of
certain macro-expansions that differ between device and host, however
wording is being put into the SYCL standard to make those cases UB.

This DOES have one bit of fallout, which is to make sure that the name
on a Windows host does NOT have its lambda mangling 'number', which we
are doing by storing the value in the Device Lamba Mangling Number
(previously done for CUDA only for similar reasons!).

Additionally, this patch removes the 'mark_kernel_name' builtin, as this
results in it being a no-op.

Author: Erich Keane

clang/docs/LanguageExtensions.rst

@@ -2521,12 +2521,9 @@ it in an instantiation which changes its value.
 In order to produce the unique name, the current implementation of the bultin
 uses Itanium mangling even if the host compilation uses a different name
 mangling scheme at runtime. The mangler marks all the lambdas required to name
-the SYCL kernel and emits a stable local ordering of the respective lambdas,
-starting from ``10000``. The initial value of ``10000`` serves as an obvious
-differentiator from ordinary lambda mangling numbers but does not serve any
-other purpose and may change in the future. The resulting pattern is
-demanglable. When non-lambda types are passed to the builtin, the mangler emits
-their usual pattern without any special treatment.
+the SYCL kernel and emits a stable local ordering of the respective lambdas.
+The resulting pattern is demanglable. When non-lambda types are passed to the
+builtin, the mangler emits their usual pattern without any special treatment.
 
 **Syntax**:
 
@@ -2556,30 +2553,6 @@ internal linkage.
   // Computes a unique stable name for a given variable.
   constexpr bool  __builtin_sycl_unique_stable_id( expr );
 
-``__builtin_sycl_mark_kernel_name``
------------------------------------
-
-``__builtin_sycl_mark_kernel_name`` is a builtin that can be used with
-``__builtin_sycl_unique_stable_name`` to make sure a kernel is properly 'marked'
-as a kernel without having to instantiate a sycl_kernel function. Typically,
-``__builtin_sycl_unique_stable_name`` can only be called in a constant expression
-context after any kernels that would change the output have been instantiated.
-This is necessary, as changing the answer to the constant expression after
-evaluation isn't permitted.  However, in some cases it can be useful to query the
-result of ``__builtin_unique_stable_name`` after we know that the name is a kernel
-name, but before we are able to instantiate the kernel itself (such as when trying
-to decide between two signatures at compile time). In these cases,
-``__builtin_sycl_mark_kernel_name`` can be used to mark the type as a kernel name,
-ensuring that ``__builtin_unique_stable_name`` gives the correct result despite the
-kernel not yet being instantiated.
-
-**Syntax**:
-
-.. code-block:: c++
-
-  // Marks a type as the name of a sycl kernel.
-  constexpr bool  __builtin_sycl_mark_kernel_name( type-id );
-
 Multiprecision Arithmetic Builtins
 ----------------------------------
 

clang/include/clang/AST/ASTContext.h

@@ -3238,32 +3238,10 @@ OPT_LIST(V)
 
   StringRef getCUIDHash() const;
 
-  void AddSYCLKernelNamingDecl(const CXXRecordDecl *RD);
-  bool IsSYCLKernelNamingDecl(const NamedDecl *RD) const;
-  unsigned GetSYCLKernelNamingIndex(const NamedDecl *RD);
-  /// A SourceLocation to store whether we have evaluated a kernel name already,
-  /// and where it happened.  If so, we need to diagnose an illegal use of the
-  /// builtin. This should only contain SYCLUniqueStableNameExprs and
-  /// SYCLUniqueStableIdExprs.
-  llvm::MapVector<const Expr *, std::string>
-      SYCLUniqueStableNameEvaluatedValues;
-
 private:
   /// All OMPTraitInfo objects live in this collection, one per
   /// `pragma omp [begin] declare variant` directive.
   SmallVector<std::unique_ptr<OMPTraitInfo>, 4> OMPTraitInfoVector;
-
-  /// A list of the (right now just lambda decls) declarations required to
-  /// name all the SYCL kernels in the translation unit, so that we can get the
-  /// correct kernel name, as well as implement
-  /// __builtin_sycl_unique_stable_name.
-  llvm::DenseMap<const DeclContext *,
-                 llvm::SmallPtrSet<const CXXRecordDecl *, 4>>
-      SYCLKernelNamingTypes;
-  std::unique_ptr<ItaniumMangleContext> SYCLKernelFilterContext;
-  void FilterSYCLKernelNamingDecls(
-      const CXXRecordDecl *RD,
-      llvm::SmallVectorImpl<const CXXRecordDecl *> &Decls);
 };
 
 /// Insertion operator for diagnostics.

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -6452,11 +6452,6 @@ def warn_gnu_null_ptr_arith : Warning<
 def warn_pointer_sub_null_ptr : Warning<
   "performing pointer subtraction with a null pointer %select{has|may have}0 undefined behavior">,
   InGroup<NullPointerSubtraction>, DefaultIgnore;
-def err_kernel_invalidates_sycl_unique_stable_name
-    : Error<"kernel %select{naming|instantiation}0 changes the result of an "
-            "evaluated '__builtin_sycl_unique_stable_%select{name|id}1'">;
-def note_sycl_unique_stable_name_evaluated_here
-    : Note<"'__builtin_sycl_unique_stable_%select{name|id}0' evaluated here">;
 def err_unique_stable_id_expected_var : Error<"expected variable name">;
 def err_unique_stable_id_global_storage
     : Error<"argument passed to '__builtin_sycl_unique_stable_id' must have "

clang/include/clang/Basic/TokenKinds.def

@@ -711,8 +711,6 @@ KEYWORD(__builtin_available              , KEYALL)
 KEYWORD(__builtin_sycl_unique_stable_name, KEYSYCL)
 KEYWORD(__builtin_sycl_unique_stable_id  , KEYSYCL)
 
-TYPE_TRAIT_1(__builtin_sycl_mark_kernel_name, SYCLMarkKernelName, KEYSYCL)
-
 // Clang-specific keywords enabled only in testing.
 TESTING_KEYWORD(__unknown_anytype , KEYALL)
 

clang/include/clang/Sema/Sema.h

@@ -1094,15 +1094,6 @@ class Sema final {
     OpaqueParser = P;
   }
 
-  // Marks a type as a SYCL Kernel without necessarily adding it.  Additionally,
-  // it diagnoses if this causes any of the evaluated
-  // __builtin_sycl_unique_stable_name values to change.
-  void MarkSYCLKernel(SourceLocation NewLoc, QualType Ty, bool IsInstantiation);
-  // Does the work necessary to deal with a SYCL kernel lambda. At the moment,
-  // this just marks the list of lambdas required to name the kernel. It does
-  // this by dispatching to MarkSYCLKernel, so it also does the diagnostics.
-  void AddSYCLKernelLambda(const FunctionDecl *FD);
-
   class DelayedDiagnostics;
 
   class DelayedDiagnosticsState {

clang/lib/AST/ASTContext.cpp

@@ -11817,86 +11817,3 @@ StringRef ASTContext::getCUIDHash() const {
   CUIDHash = llvm::utohexstr(llvm::MD5Hash(LangOpts.CUID), /*LowerCase=*/true);
   return CUIDHash;
 }
-
-// Get the closest named parent, so we can order the sycl naming decls somewhere
-// that mangling is meaningful.
-static const DeclContext *GetNamedParent(const CXXRecordDecl *RD) {
-  const DeclContext *DC = RD->getDeclContext();
-
-  while (!isa<NamedDecl, TranslationUnitDecl>(DC))
-    DC = DC->getParent();
-  return DC;
-}
-
-void ASTContext::AddSYCLKernelNamingDecl(const CXXRecordDecl *RD) {
-  assert(getLangOpts().isSYCL() && "Only valid for SYCL programs");
-  RD = RD->getCanonicalDecl();
-  const DeclContext *DC = GetNamedParent(RD);
-
-  assert(RD->getLocation().isValid() &&
-         "Invalid location on kernel naming decl");
-
-  (void)SYCLKernelNamingTypes[DC].insert(RD);
-}
-
-bool ASTContext::IsSYCLKernelNamingDecl(const NamedDecl *ND) const {
-  assert(getLangOpts().isSYCL() && "Only valid for SYCL programs");
-  const auto *RD = dyn_cast<CXXRecordDecl>(ND);
-  if (!RD)
-    return false;
-  RD = RD->getCanonicalDecl();
-  const DeclContext *DC = GetNamedParent(RD);
-
-  auto Itr = SYCLKernelNamingTypes.find(DC);
-
-  if (Itr == SYCLKernelNamingTypes.end())
-    return false;
-
-  return Itr->getSecond().count(RD);
-}
-
-// Filters the Decls list to those that share the lambda mangling with the
-// passed RD.
-void ASTContext::FilterSYCLKernelNamingDecls(
-    const CXXRecordDecl *RD,
-    llvm::SmallVectorImpl<const CXXRecordDecl *> &Decls) {
-
-  if (!SYCLKernelFilterContext)
-    SYCLKernelFilterContext.reset(
-        ItaniumMangleContext::create(*this, getDiagnostics()));
-
-  llvm::SmallString<128> LambdaSig;
-  llvm::raw_svector_ostream Out(LambdaSig);
-  SYCLKernelFilterContext->mangleLambdaSig(RD, Out);
-
-  llvm::erase_if(Decls, [this, &LambdaSig](const CXXRecordDecl *LocalRD) {
-    llvm::SmallString<128> LocalLambdaSig;
-    llvm::raw_svector_ostream LocalOut(LocalLambdaSig);
-    SYCLKernelFilterContext->mangleLambdaSig(LocalRD, LocalOut);
-    return LambdaSig != LocalLambdaSig;
-  });
-}
-
-unsigned ASTContext::GetSYCLKernelNamingIndex(const NamedDecl *ND) {
-  assert(getLangOpts().isSYCL() && "Only valid for SYCL programs");
-  assert(IsSYCLKernelNamingDecl(ND) &&
-         "Lambda not involved in mangling asked for a naming index?");
-
-  const CXXRecordDecl *RD = cast<CXXRecordDecl>(ND)->getCanonicalDecl();
-  const DeclContext *DC = GetNamedParent(RD);
-
-  auto Itr = SYCLKernelNamingTypes.find(DC);
-  assert(Itr != SYCLKernelNamingTypes.end() && "Not a valid DeclContext?");
-
-  const llvm::SmallPtrSet<const CXXRecordDecl *, 4> &Set = Itr->getSecond();
-
-  llvm::SmallVector<const CXXRecordDecl *> Decls{Set.begin(), Set.end()};
-
-  FilterSYCLKernelNamingDecls(RD, Decls);
-
-  llvm::sort(Decls, [](const CXXRecordDecl *LHS, const CXXRecordDecl *RHS) {
-    return LHS->getLambdaManglingNumber() < RHS->getLambdaManglingNumber();
-  });
-
-  return llvm::find(Decls, RD) - Decls.begin();
-}

clang/lib/AST/Expr.cpp

@@ -541,27 +541,17 @@ std::string SYCLUniqueStableNameExpr::ComputeName(ASTContext &Context) const {
                                                getTypeSourceInfo()->getType());
 }
 
-static llvm::Optional<unsigned> SYCLMangleCallback(ASTContext &Ctx,
-                                                   const NamedDecl *ND) {
-  // This replaces the 'lambda number' in the mangling with a unique number
-  // based on its order in the declaration.  To provide some level of visual
-  // notability (actual uniqueness from normal lambdas isn't necessary, as
-  // these are used differently), we add 10,000 to the number.
-  // For example:
-  // _ZTSZ3foovEUlvE10005_
-  // Demangles to: typeinfo name for foo()::'lambda10005'()
-  // Note that the mangler subtracts 2, since with normal lambdas the lambda
-  // mangling number '0' is an anonymous struct mangle, and '1' is omitted.
-  // So 10,002 results in the first number being 10,000.
-  if (Ctx.IsSYCLKernelNamingDecl(ND))
-    return 10'002 + Ctx.GetSYCLKernelNamingIndex(ND);
+static llvm::Optional<unsigned>
+UniqueStableNameDiscriminator(ASTContext &, const NamedDecl *ND) {
+  if (const auto *RD = dyn_cast<CXXRecordDecl>(ND))
+    return RD->getDeviceLambdaManglingNumber();
   return llvm::None;
 }
 
 std::string SYCLUniqueStableNameExpr::ComputeName(ASTContext &Context,
                                                   QualType Ty) {
   std::unique_ptr<MangleContext> Ctx{ItaniumMangleContext::create(
-      Context, Context.getDiagnostics(), SYCLMangleCallback)};
+      Context, Context.getDiagnostics(), UniqueStableNameDiscriminator)};
 
   std::string Buffer;
   Buffer.reserve(128);
@@ -612,7 +602,7 @@ std::string SYCLUniqueStableIdExpr::ComputeName(ASTContext &Context) const {
 std::string SYCLUniqueStableIdExpr::ComputeName(ASTContext &Context,
                                                 const VarDecl *VD) {
   std::unique_ptr<MangleContext> Ctx{ItaniumMangleContext::create(
-      Context, Context.getDiagnostics(), SYCLMangleCallback)};
+      Context, Context.getDiagnostics(), UniqueStableNameDiscriminator)};
 
   std::string Buffer;
   Buffer.reserve(128);

clang/lib/AST/ExprConstant.cpp

@@ -8674,8 +8674,6 @@ class PointerExprEvaluator
   bool VisitSYCLUniqueStableNameExpr(const SYCLUniqueStableNameExpr *E) {
     std::string ResultStr = E->ComputeName(Info.Ctx);
 
-    Info.Ctx.SYCLUniqueStableNameEvaluatedValues[E] = ResultStr;
-
     QualType CharTy = Info.Ctx.CharTy.withConst();
     APInt Size(Info.Ctx.getTypeSize(Info.Ctx.getSizeType()),
                ResultStr.size() + 1);
@@ -8694,8 +8692,6 @@ class PointerExprEvaluator
   bool VisitSYCLUniqueStableIdExpr(const SYCLUniqueStableIdExpr *E) {
     std::string ResultStr = E->ComputeName(Info.Ctx);
 
-    Info.Ctx.SYCLUniqueStableNameEvaluatedValues[E] = ResultStr;
-
     QualType CharTy = Info.Ctx.CharTy.withConst();
     APInt Size(Info.Ctx.getTypeSize(Info.Ctx.getSizeType()),
                ResultStr.size() + 1);

clang/lib/AST/ItaniumCXXABI.cpp

@@ -181,6 +181,37 @@ class ItaniumNumberingContext : public MangleNumberingContext {
   }
 };
 
+// A version of this for SYCL that makes sure that 'device' mangling context
+// matches the lambda mangling number, so that __builtin_sycl_unique_stable_name
+// can be consistently generated between a MS and Itanium host by just referring
+// to the device mangling number.
+class ItaniumSYCLNumberingContext : public ItaniumNumberingContext {
+  llvm::DenseMap<const CXXMethodDecl *, unsigned> ManglingNumbers;
+  using ManglingItr = decltype(ManglingNumbers)::iterator;
+
+public:
+  ItaniumSYCLNumberingContext(ItaniumMangleContext *Mangler)
+      : ItaniumNumberingContext(Mangler) {}
+
+  unsigned getManglingNumber(const CXXMethodDecl *CallOperator) override {
+    unsigned Number = ItaniumNumberingContext::getManglingNumber(CallOperator);
+    std::pair<ManglingItr, bool> emplace_result =
+        ManglingNumbers.try_emplace(CallOperator, Number);
+    (void)emplace_result;
+    assert(emplace_result.second && "Lambda number set multiple times?");
+    return Number;
+  }
+
+  using ItaniumNumberingContext::getManglingNumber;
+
+  unsigned getDeviceManglingNumber(const CXXMethodDecl *CallOperator) override {
+    ManglingItr Itr = ManglingNumbers.find(CallOperator);
+    assert(Itr != ManglingNumbers.end() && "Lambda not yet mangled?");
+
+    return Itr->second;
+  }
+};
+
 class ItaniumCXXABI : public CXXABI {
 private:
   std::unique_ptr<MangleContext> Mangler;
@@ -249,6 +280,9 @@ class ItaniumCXXABI : public CXXABI {
 
   std::unique_ptr<MangleNumberingContext>
   createMangleNumberingContext() const override {
+    if (Context.getLangOpts().isSYCL())
+      return std::make_unique<ItaniumSYCLNumberingContext>(
+          cast<ItaniumMangleContext>(Mangler.get()));
     return std::make_unique<ItaniumNumberingContext>(
         cast<ItaniumMangleContext>(Mangler.get()));
   }

clang/lib/AST/ItaniumMangle.cpp

@@ -1518,9 +1518,16 @@ void CXXNameMangler::mangleUnqualifiedName(GlobalDecl GD,
     // <lambda-sig> ::= <template-param-decl>* <parameter-type>+
     //     # Parameter types or 'v' for 'void'.
     if (const CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(TD)) {
-      if (Record->isLambda() && (Record->getLambdaManglingNumber() ||
-                                 Context.getDiscriminatorOverride()(
-                                     Context.getASTContext(), Record))) {
+      llvm::Optional<unsigned> DeviceNumber =
+          Context.getDiscriminatorOverride()(Context.getASTContext(), Record);
+
+      // If we have a device-number via the discriminator, use that to mangle
+      // the lambda, otherwise use the typical lambda-mangling-number. In either
+      // case, a '0' should be mangled as a normal unnamed class instead of as a
+      // lambda.
+      if (Record->isLambda() &&
+          ((DeviceNumber && *DeviceNumber > 0) ||
+           (!DeviceNumber && Record->getLambdaManglingNumber() > 0))) {
         assert(!AdditionalAbiTags &&
                "Lambda type cannot have additional abi tags");
         mangleLambda(Record);
@@ -1960,8 +1967,8 @@ void CXXNameMangler::mangleLambda(const CXXRecordDecl *Lambda) {
   // mangling number for this lambda.
   llvm::Optional<unsigned> DeviceNumber =
       Context.getDiscriminatorOverride()(Context.getASTContext(), Lambda);
-  unsigned Number = DeviceNumber.hasValue() ? *DeviceNumber
-                                            : Lambda->getLambdaManglingNumber();
+  unsigned Number =
+      DeviceNumber ? *DeviceNumber : Lambda->getLambdaManglingNumber();
 
   assert(Number > 0 && "Lambda should be mangled as an unnamed class");
   if (Number > 1)