[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/test/CodeGenSYCL/unique_stable_name.cpp:86-89
+  // FIXME: Ensure that j is incremented because VLAs are terrible.
+  int j = 55;
+  puts(__builtin_sycl_unique_stable_name(int[++j]));
+  // CHECK: call spir_func void @puts(i8 addrspace(4)* addrspacecast (i8* getelementptr inbounds ([[STRING_SIZE]], [[STRING_SIZE]]* @[[STRING]], i32 0, i32 0) to i8 addrspace(4)*))
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > @rjmccall  -- any opinions or ideas on this? I think VLAs should likely behave the same as they do in `sizeof`, etc.
> > > VLA side-effects are a bit of an edge case in the language that I think are an unfortunate 'gotcha'.  
> > > 
> > > I'm not sure where I fall on this in general (other than hating it as a 'feature' of C), but I'd hope that this is something we can leave as a FIXME, as this is a builtin for the use of a library.  I don't suspect that it will affect the normal uses of this builtin at all, so I hope it is something we look into once someone actually cares.
> > My feeling is: I think we should support the usual VLA semantics if SYCL supports VLAs in any way. If it's easy to do so as part of this patch then great, but if it continues to elude us, I'm totally fine doing it as a follow-up because this is definitely a wonky edge case.
> I looked into it at one point, and it seems that the VLA handling in sizeof (and similar builtins) is done quite manually in quite a few places, it seemed like a pretty involved amount of work.  I'd still hope to do it as a part of a follow-up (keyed by a user mentioning they care).
> 
> So I checked up on this... 
> 1- The SYCL "Language" spec doesn't support VLAs at all (the same way C++ doesnt :)).
> 
> 2- In our to-be-upstreamed downstream, we enforce the SYCL language rule that VLAs aren't allowed to be passed to kernels as they aren't a sized type.
> 
> 3- The offload 'spir' target that is used for SYCL ALSO disallows VLAs with the error: error: variable length arrays are not supported for the current target
> 
> So I think it would be quite a bit of twisting for someone to get this builtin to apply to a VLA.  WDYT?
> So I think it would be quite a bit of twisting for someone to get this builtin to apply to a VLA. WDYT?

Sold on leaving this as a FIXME until someone finds an actual issue they care about. Thanks for checking on all this!


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

https://reviews.llvm.org/D103112