[PATCH] D103112: Reimplement __builtin_unique_stable_name-

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:2413
+(or type of the expression) that is stable across split compilations, mainly to
+support SYCL/Data Parallel C++ language.
+
----------------
The semantics here seem specific to SYCL.  In fact, if this feature were used in normal code, it would arguably be weird that it changed behavior significantly in SYCL.  I think we should just acknowledge that and make it a feature that's only enabled when SYCL is enabled.  That probably also means renaming it to `__builtin_sycl_unique_stable_name`.


================
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
----------------
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?


================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1965
+  // UniqueStableNameExpr
+  EXPR_UNIQUESTABLENAME,
 };
----------------
UniQuestableName :)

Probably ought to use underscores here.


================
Comment at: clang/lib/AST/ASTContext.cpp:11662
+
+void ASTContext::AddSYCLKernelNamingDecl(const CXXRecordDecl *RD) {
+  RD = RD->getCanonicalDecl();
----------------
Can we assert that these methods are only called in SYCL?  I'd hate to accidentally do this bookkeeping in other modes.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1977
+  if (Context.getShouldCallKernelCallback()(Context.getASTContext(), Lambda)) {
+    Context.getKernelMangleCallback()(Context.getASTContext(), Lambda, Out);
+    Out << '_';
----------------
This basically assumes that the callback is only changing the discriminator.  And in fact, we already have this "device lambda mangling number" concept that we use in different modes with similar problems to SYCL.  Can we unify these and just provide one way for the context to opt in to overriding discriminators?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5056
+
+    Out << "u20__unique_stable_name";
+    if (USN->isExpr()) {
----------------
The expectation is that these names match the spelling in the source, so this should include the `__builtin` prefix.


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

https://reviews.llvm.org/D103112