[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

[Erich Keane via Phabricator via cfe-commits]

erichkeane 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.
+
----------------
rjmccall wrote:
> 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`.
We can live with that, that makes sense to me. 


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1977
+  if (Context.getShouldCallKernelCallback()(Context.getASTContext(), Lambda)) {
+    Context.getKernelMangleCallback()(Context.getASTContext(), Lambda, Out);
+    Out << '_';
----------------
rjmccall wrote:
> 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?
I was unable to find a way to get the device lambda mangling number to work in this situation unfortunately, it seems to have significantly different needs from what we need here.  

Part of what SYCL needs is the ability to 'recalculate' this number as we discover that a lambda is participating in naming a SYCL kernel. The DeviceLambdaMangling mechanism requires that it be evaluated as we are generating the lambdas.  I couldn't find a mechanism to update them after the fact that wasn't messier than the callback mechanism.

As far as assuming that we are changing only the discriminator, that ends up being required since this is the only location where a lambda mangling is 'customizable', and we want it to remain demanglable.



================
Comment at: clang/lib/Basic/IdentifierTable.cpp:85
 
-  enum {
-    KEYC99        = 0x1,
-    KEYCXX        = 0x2,
-    KEYCXX11      = 0x4,
-    KEYGNU        = 0x8,
-    KEYMS         = 0x10,
-    BOOLSUPPORT   = 0x20,
-    KEYALTIVEC    = 0x40,
-    KEYNOCXX      = 0x80,
-    KEYBORLAND    = 0x100,
-    KEYOPENCLC    = 0x200,
-    KEYC11        = 0x400,
-    KEYNOMS18     = 0x800,
-    KEYNOOPENCL   = 0x1000,
-    WCHARSUPPORT  = 0x2000,
-    HALFSUPPORT   = 0x4000,
-    CHAR8SUPPORT  = 0x8000,
-    KEYCONCEPTS   = 0x10000,
-    KEYOBJC       = 0x20000,
-    KEYZVECTOR    = 0x40000,
-    KEYCOROUTINES = 0x80000,
-    KEYMODULES    = 0x100000,
-    KEYCXX20      = 0x200000,
-    KEYOPENCLCXX  = 0x400000,
-    KEYMSCOMPAT   = 0x800000,
-    KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
-    KEYALL = (0xffffff & ~KEYNOMS18 &
-              ~KEYNOOPENCL) // KEYNOMS18 and KEYNOOPENCL are used to exclude.
-  };
-
-  /// How a keyword is treated in the selected standard.
-  enum KeywordStatus {
-    KS_Disabled,    // Disabled
-    KS_Extension,   // Is an extension
-    KS_Enabled,     // Enabled
-    KS_Future       // Is a keyword in future standard
-  };
+enum {
+  KEYC99 = 0x1,
----------------
aaron.ballman wrote:
> It looks like you formatted a bit more than you intended to.
Ugh... tis what I get for running clang-format on the whole patch :/


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

https://reviews.llvm.org/D103112