erichkeane 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)*)) ---------------- 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? 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
