Ah, yes, thanks! That shows the OTHER interesting thing about lambda mangling: The ‘0th’ is mangled as ‘anonymous struct’, the ones after that are mangled as N-1. I was wondering where that went in my example…
It is that “somehow” that is the challenge and that I am lacking good concrete ideas for.
I think a stable name approach is still likely the best approach though it very well might be possible that any scheme can be broken with enough macro and template shenanigans. The challenge is drawing the line where arguably reasonable macro use crosses into just wanting to watch the world burn territory.
An AST pattern matching approach sounds to me more likely to result in mismatches and, when that happens, diagnostics and debugging seem likely to be substantially more challenging then analyzing why two names differ.
Honestly, the string-matching across two compiler invocations seems significantly more difficult to me than some level of AST matching. Depending on how much you’re willing to mandate is ‘the same’. For one, you can require the same line numbers consistently without having to play with macros.
You could also check the calling-function and contents of the lambda, the three of which combined end up being pretty bullet proof.
Using a demangle-able name is still a ‘nice’ idea and I wouldn’t discourage it, but matching the two by name alone has already shown itself to be pretty error-prone (And frankly, after YEARS of beating it into shape, we still got bug reports that we failed to match correctly).
Thank you for this RFC and the good discussion on it. From what I can tell, the first two bullets are obviating by the decision to not support arbitrary third-party host compilers, and there is still some discomfort around __builtin_sycl_unique_stable_name but a clearly better approach has not been suggested. It’s not quite consensus to move forward, but it’s also not consensus against moving forward; more that “the devil is in the details.” I think you’re clear to move forward with this design approach, but please keep an open mind during the review discussions as a better approach might surface through that process.
Please be sure to add the following people to code reviews in this area unless they say they don’t feel they need to be involved: @ErichKeane, @jdoerfert.
I have no problem enhancing this feature, though it is only necessary because of integration headers/footers. IMO, we would be better served using a different mechanism for matching of the kernels (see how OpenMP offload is doing it), and removing this builtin entirely.
IMO, we would be better served using a different mechanism for matching of the kernels (see how OpenMP offload is doing it), and removing this builtin entirely.
Should an alternate mechanism be identified and implemented, removal of the builtin should be communicated and coordinated with maintainers of downstream projects that use it (e.g., by AdaptiveCpp; https://github.com/AdaptiveCpp/AdaptiveCpp/blob/75783024706c17142e2808d633340d796b3bd6dc/include/hipSYCL/glue/ze/ze_kernel_launcher.hpp#L389-L395).
Yep! I’m aware of that one, and realize it is probably evidence that we’ll never get rid of the builtin, but at least the dependence on it for our SYCL implementation could/should be removed.
Thanks for checking on our use; the usage of that builtin in our code base is part of earlier experiments where we tried to reuse portions of upstreamed SYCL support. Those code parts have been deprecated for quite some time. It’s not part of any production code paths. Of course, our most important compilation flow, the single pass compiler, does not need such builtins.
From our side there is no dependency on this builtin.
2 Likes
Thats very interesting feedback! Thanks for that!
1 Like