[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

Erich Keane via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 26 11:45:48 PDT 2021


erichkeane added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:2443
+  // Computes a unique stable name for the type of the given expression.
+  constexpr const char * __builtin_unique_stable_name( expression );
+
----------------
rjmccall wrote:
> These need to be updated for the rename.  You should just grep the patch for the old name.
Looks like there was 1 more in addition to this that Aaron hadn't found! It'll be fixed in the next version.


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


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1977
+  if (Context.getShouldCallKernelCallback()(Context.getASTContext(), Lambda)) {
+    Context.getKernelMangleCallback()(Context.getASTContext(), Lambda, Out);
+    Out << '_';
----------------
rjmccall wrote:
> erichkeane wrote:
> > 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.
> > 
> Sorry, I didn't mean that you should try to make the SYCL logic just set a device mangling number; in fact, I meant almost the reverse.  The device mangling number is ultimately a MangleContext-driven override of the discriminator choice, just like you're trying to add for SYCL.  For SYCL, you're adding a generalized callback mechanism, which seems good.  What I'm asking is that you go ahead and move  the existing device-mangling logic in the mangler over to that callback mechanism, so that instead of setting an `isDeviceMangleContext()` bit on the MangleContext, that code will install an discriminator-override callback that returns the device lambda mangling number.  Then we have one mechanism instead of two.
> 
> I think the right API for that callback is just to have it return an `Optional<unsigned>`, and then you use the normal discriminator if it returns `None`.  And it should take an arbitrary `Decl*` so that it can override discriminators on non-lambda local declarations if it wants.
I think that should work... I'll look into it, thanks for the clarification!


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

https://reviews.llvm.org/D103112



More information about the cfe-commits mailing list