[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, that seems to work out cleanly.



================
Comment at: clang/include/clang/AST/Expr.h:2045
+// representation of the type (or type of the expression) in a way that permits
+// us to properly encode information about the SYCL kernels.
+class UniqueStableNameExpr final
----------------
erichkeane wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > Since this is really just for internal use in system headers (right?), is there a need for it to be as flexible as this about whether it takes an expression or a type?
> > That said, I don't really have a strong objection to supporting either a type or an expression operand.
> I had responded to this I thought?  I found no good reason to do expression, we can sprinkle decltype around to deal with that, I'll prep a patch to remove the expr bits.
Okay.  So this doesn't really need trailing storage anymore, and the documentation needs to be updated to not mention the expression production.


================
Comment at: clang/include/clang/AST/Mangle.h:174
+  using KernelMangleCallbackTy =
+      llvm::Optional<unsigned> (*)(ASTContext &, const CXXRecordDecl *);
   explicit ItaniumMangleContext(ASTContext &C, DiagnosticsEngine &D)
----------------
The concept here isn't kernel-specific, and it's not an arbitrary callback.  I think it would be better to call this something more generic, like DiscriminatorOverride.

Should the argument have to be a record?  Other local declarations can show up as e.g. template arguments, like enums or (I think) static local variables.


================
Comment at: clang/lib/AST/ASTContext.cpp:11689
+  assert((getLangOpts().SYCLIsDevice || getLangOpts().SYCLIsHost) &&
+         "Only valid for SYCL programs");
+  RD = RD->getCanonicalDecl();
----------------
Could you add an `isSYCL()` method on LangOpts as a convenience for this?


================
Comment at: clang/lib/Basic/IdentifierTable.cpp:159
     return KS_Future;
+  if ((LangOpts.SYCLIsDevice || LangOpts.SYCLIsHost) && Flags & KEYSYCL)
+    return KS_Enabled;
----------------
I think people would generally happier if you parenthesized `Flags & KEYSYCL`, even though, yes, this does work under C precedence rules.


================
Comment at: clang/lib/CodeGen/CGCUDANV.cpp:196
+  // If the host and device have different C++ ABIs, mark it as the device
+  // mangle context so that the mangling needs to retrieve the additonal
+  // device lambda mangling number instead of the regular host one.
----------------
typo: additonal


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103112/new/

https://reviews.llvm.org/D103112