[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

Mon Jul 31 13:14:26 PDT 2023

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