[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D76620#2328305 <https://reviews.llvm.org/D76620#2328305>, @rjmccall wrote:

> In D76620#2328031 <https://reviews.llvm.org/D76620#2328031>, @erichkeane wrote:
>
>> In D76620#2328011 <https://reviews.llvm.org/D76620#2328011>, @rjmccall wrote:
>>
>>> In D76620#2327988 <https://reviews.llvm.org/D76620#2327988>, @erichkeane wrote:
>>>
>>>> In D76620#2327976 <https://reviews.llvm.org/D76620#2327976>, @rjmccall wrote:
>>>>
>>>>> In D76620#2327910 <https://reviews.llvm.org/D76620#2327910>, @erichkeane wrote:
>>>>>
>>>>>> In D76620#2327901 <https://reviews.llvm.org/D76620#2327901>, @rjmccall wrote:
>>>>>>
>>>>>>> You know on both sides that a lambda is used as a kernel, yes?  Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?
>>>>>>
>>>>>> Coming up with said stable sequence is somewhat difficult as well.  A strict integer ordering didn't end up being stable, as some of the kernel handling can cause instantiations to happen in a slightly different ordering, which messed that up, as would use of the builtin. We wanted to find a way that was dependent on the source code alone.  The "Quick and Dirty" solution was line/column numbers, though we considered a hash of that same information to at least make it shorter.
>>>>>
>>>>> Certainly you wouldn't want a *global* sequence ID.  However, lambdas can't just occur globally, they're always part of some declaration that they can be scoped to, so that you have e.g. "the third kernel lambda within function X".  I fail to see how that wouldn't address the concern about instantiation order, and it's still source-directed.
>>>>
>>>> Hmm... I'll have to consider that.  That is an interesting thought. Presumably we could just copy the 'getLambdaManglingNumber' stuff in that case, and place the value in the same location in the mangling, with some sort of discriminator (to avoid conflicting manglings).
>>>
>>> The Itanium mangling already produces a different lambda mangling number for different lambda signatures.  You just need the kernel-ness to be part of the signature.
>>
>> It does it via the getLambdaManglingNumber I believe, right?  The idea would be to have a getKernelLambdaManglingNumber that does something similar, and needs something to avoid conflicting, which is the discriminator I was mentioning.
>
> Right, so if you look how that's ultimately derived in ItaniumNumberingContext::getManglingNumber, you'll see that every context has different sequences for unique lambda-sigs.  If you can just put "this is a kernel" into the lambda-sig, you'll automatically get a stable sequence for kernel lambdas.  But that would rely on immediately knowing that a lambda will be used as a kernel.
>
>>>> The only other concern I have is WHEN we know that a lambda is used in a kernel, which we don't until it is called (and can cause confusion when it is a template parameter and called later).
>>>
>>> Oh, yes, if you don't know that a lambda is used as a kernel locally then this falls apart a bit.  You could of course pretend for ABI purposes that there's a new intermediate lambda at the kernel use point when the lambda is not local to the current function.  I don't think there's anything which relies on mangling lambdas before the function they're contained within is fully type-checked.
>>
>> One thing that might save us from this, is we aren't actually modifying the LAMBDAs mangling, we're modifying the KERNEL's mangling. The lambda itself keeps its mangling, but the kernel that calls it (which is the device/host boundary) is what has its name changed.  So at least at that point we know some about it.  And, actually, since we are generating the 'integeration header' lookup table AND the kernel in identical (if not the same) invocation, perhaps a global counter WOULD work...  My coworker is investigating this now, so hopefully she'll be able to prove out this idea and give further feedback here.
>
> I feel obliged to point out that, if you're worried about a single function potentially having a different set of lambdas in different translation modes, that's also going to affect things like template specialization identity.  That is, suppose a kernel function is formed for a particular lambda, and in one translation mode it's the first lambda with that signature in a function and in another mode it's the second.  That's going to change the immediate mangling of the lambda, but it's also going to change the mangling of templates used with that lambda.  For example, if you have

We typically disallow the kernel itself having a different lambda in each mode, as that doesn't make much sense.  The SYCL language is a little coy with the ODR when it comes to macros, but typically the kernel calls themselves have to be consistent in a single application.

>   template <class F> void run_wrapped_as_kernel(const F &f) {
>     use_as_kernel([=] { f(); });  // this lambda is always the first in its context, but the identity of that context is contingent on something unstable
>   }
>   
>   void foo() {
>   #if mode1
>     auto blah = []{}; // a lambda that's only present in one language mode
>   #endif
>     use_as_kernel([] { ... }); // the mangling number for this is disturbed
>   }
>
> So if you're going to try to solve this (instead of just making it the user's responsibility to not do), it's almost like you need some way to retroactively number every lambda implicated in any way by the host/kernel interface, and then have a way to mangle things completely from scratch using those stable numberings for this kernel-numbering thing.

THAT right there is exactly the problem we ran into, the 'blah' changing the kernel numbering. Our previous 'retroactive way' of numbering was line&column numbers + the same for macro invocations as it was stable with that. Mangling them retroactively is perhaps a possibility, though it would be great if we could force a number based on textual order (complicated further by template instantiations).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76620