rjmccall 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 );
+
----------------
These need to be updated for the rename. You should just grep the patch for
the old 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
----------------
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.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1977
+ if (Context.getShouldCallKernelCallback()(Context.getASTContext(), Lambda)) {
+ Context.getKernelMangleCallback()(Context.getASTContext(), Lambda, Out);
+ Out << '_';
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103112/new/
https://reviews.llvm.org/D103112
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
