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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
