[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/include/clang/Basic/LangOptions.h:448
+
+  bool isSYCL() const { return SYCLIsDevice || SYCLIsHost; }
 };
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > FWIW, we also have `SYCLVersion != SYCL_None` as a possible way to express this. Perhaps we should just use that anywhere we're using `SYCLIsDevice || SYCLIsHost` currently? (I don't have a strong opinion but am bringing it up in case someone else does.)
> IMO, that is a fairly 'indirect' mechanism for it. I think host+device is the 'right' way to do it, an d I think the SYCLVersion is a 'consequence' of those.
> 
> It is unfortunate that SYCL forces us to have 3 things to express in language opts (vs 2 for the other languages).
> 
That's a fair point -- I'm happy with either approach. Using `isSYCL()` is similarly expressive but with less typing, so I'm fine with it.


================
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:
> > @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.


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

https://reviews.llvm.org/D103112