AlexVlx added a comment. In D156539#4547814 <https://reviews.llvm.org/D156539#4547814>, @rjmccall wrote:
> In D156539#4546834 <https://reviews.llvm.org/D156539#4546834>, @AlexVlx wrote: > >> In D156539#4542836 <https://reviews.llvm.org/D156539#4542836>, @rjmccall >> wrote: >> >>> We should probably write this code to work properly in case we add a target >>> that makes `__builtin_alloca` return a pointer in the private address >>> space. Could you recover the target AS from the type of the expression >>> instead of assuming `LangAS::Default`? >> >> I believe it should work as written (it is possible that I misunderstand the >> objection, case in which I do apologise). The issue is precisely that for >> some targets (amdgcn being one) `alloca` returns a pointer to private, which >> doesn't compose with the default AS being unspecified / nonexistent, for >> some languages (e.g. HIP, C/C++ etc.). For the OCL case @arsenm mentions, I >> believe that we make a choice based on `LangOpts.OpenCLGenericAddressSpace`, >> and the code above would only be valid from the OCL language perspective iff >> that is true. I've put together a short Godbolt that captures these (and the >> bug the patch should fix, please see the bitcasts between ASes in the >> non-OCL case): https://gcc.godbolt.org/z/sYGK76zqv. > > An address space conversion is required in IR if there is a difference > between the address space of the pointer type formally returned by the call > to `__builtin_alloca` and the address space produced by the `alloca` > operation in IR. If Sema has set the type of `__builtin_alloca` to formally > return something in the stack address space, no conversion is required. What > I'm saying that I'd like you to not directly refer to `LangAS::Default` in > this code, because you're assuming that `__builtin_alloca` is always > returning a pointer that's formally in `LangAS::Default`. Please recover the > target address space from the type of the expression. > > Additionally, in IRGen we allow the target to hook address-space conversions; > please call `performAddrSpaceCast` instead of directly emitting an > `addrspacecast` instruction. Hmm, I think I see where you are coming from, thank you for clarifying and apologies for not seeing it from the getgo. Before delving into it, I will note that this has been handled similarly, but not identically, for static allocas for a while e.g.: https://github.com/llvm/llvm-project/blob/e0b3f45d87a6677efb59d8a4f0e6deac9c346c76/clang/lib/CodeGen/CGExpr.cpp#L83. I guess the concern here is somewhat forward looking because today the builtin is defined as `BUILTIN(__builtin_alloca, "v*z" , "Fn")`, which means that it won't carry an AS on its return value on the ast, and will only ever return a pointer to generic / default (why the query around ASTAllocaAddressSpace exists, I assume). AFAICT, we actually expect languages that are not OCL to have alloca return a pointer to LangAS::Default, please see: https://github.com/llvm/llvm-project/blob/5fbee1c6e300eee9ce9d18275bf8a6de0a22ba59/clang/lib/CodeGen/CGDecl.cpp#L1443. My sense is that, currently, on an AST level, the stack AS must be the default as or, iff OpenCL, opencl_private (also the `alloca` AS in OCL), but the alloca AS needn't be the stack AS. Which I guess could be changed in the future, but seems like it'd take some work. Granted, that's maybe not a reason to constrain the builtin, but then static and dynamic allocas will hypothetically behave differently. Noted on the target-hook, will do, thank you. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156539/new/ https://reviews.llvm.org/D156539 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
