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