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

[AlexVlx]

Summary

alloca instructions always return pointers to the stack / alloca address space. This composes poorly with most HLLs which are address space agnostic and thus have all pointers point to generic/flat. Static allocas were already handled on the AST level, in the implementation of the CreateTempAlloca family of functions, however dynamic allocas were not, which would make the following valid C++ code:

const char* p = static_cast<const char*>(__builtin_alloca(42));

lead to subtly incorrect IR / an assert flaring. This patch addresses that by inserting an address space cast iff the alloca address space is different from generic.

[rjmccall]

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?

[arsenm]

In 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?

This should happen for opencl today with amdgpu

[arsenm]

clang/lib/CodeGen/CGBuiltin.cpp

3540

No return after else

3558

No return after else

[AlexVlx]

In 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.

[AlexVlx]

Incorporated review feedback. Updated test.

[arsenm]

clang/test/CodeGen/dynamic-alloca-with-address-space.c

2

Can you add an opencl 1.2 and 2.0 run line too

13

Use generated checks

[AlexVlx]

clang/test/CodeGen/dynamic-alloca-with-address-space.c

2

This is not valid 1.2 code, for 2.0 sure.

[arsenm]

clang/test/CodeGen/dynamic-alloca-with-address-space.c

2

well also need a 1.2 flavored version then with the __privates in it (or use some macro trickery)

[rjmccall]

In D156539#4546834, @AlexVlx wrote:

In 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.

[AlexVlx]

In D156539#4547814, @rjmccall wrote:

In D156539#4546834, @AlexVlx wrote:

In 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.

[AlexVlx]

Address review comments.

[rjmccall]

clang/lib/CodeGen/CGBuiltin.cpp

3539

E->getType().getAddressSpace() is actually the top-level qualification of the type; you need E->getType()->getPointeeType().getAddressSpace().

[AlexVlx]

Use pointee's address space for the check.

[AlexVlx]

clang/lib/CodeGen/CGBuiltin.cpp

3539

D'oh! I will probably never avoid getting tripped by this, sorry for the noise.

[rjmccall]

No problem, LGTM.

clang/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_fpclassify: {
     Builder.CreateBr(End);
     Result->addIncoming(NormalResult, NotInf);
 
     // return Result
     Builder.SetInsertPoint(End);
     return RValue::get(Result);
   }
 
+  // An alloca will always return a pointer to the alloca (stack) address
+  // space. This address space need not be the same as the AST / Language
+  // default (e.g. in C / C++ auto vars are in the generic address space). At
+  // the AST level this is handled within CreateTempAlloca et al., but for the
+  // builtin / dynamic alloca we have to handle it here. We use an explicit cast
+  // instead of passing an AS to CreateAlloca so as to not inhibit optimisation.
   case Builtin::BIalloca:
   case Builtin::BI_alloca:
   case Builtin::BI__builtin_alloca_uninitialized:
   case Builtin::BI__builtin_alloca: {
     Value *Size = EmitScalarExpr(E->getArg(0));
     const TargetInfo &TI = getContext().getTargetInfo();
     // The alignment of the alloca should correspond to __BIGGEST_ALIGNMENT__.
     const Align SuitableAlignmentInBytes =
         CGM.getContext()
             .toCharUnitsFromBits(TI.getSuitableAlign())
             .getAsAlign();
     AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
     AI->setAlignment(SuitableAlignmentInBytes);
     if (BuiltinID != Builtin::BI__builtin_alloca_uninitialized)
       initializeAlloca(*this, AI, Size, SuitableAlignmentInBytes);
+    if (getASTAllocaAddressSpace() != LangAS::Default)
+      return RValue::get(Builder.CreateAddrSpaceCast(AI, CGM.Int8PtrTy));
+    else
       return RValue::get(AI);
   }
 
   case Builtin::BI__builtin_alloca_with_align_uninitialized:
   case Builtin::BI__builtin_alloca_with_align: {
     Value *Size = EmitScalarExpr(E->getArg(0));
     Value *AlignmentInBitsValue = EmitScalarExpr(E->getArg(1));
     auto *AlignmentInBitsCI = cast<ConstantInt>(AlignmentInBitsValue);
     unsigned AlignmentInBits = AlignmentInBitsCI->getZExtValue();
     const Align AlignmentInBytes =
         CGM.getContext().toCharUnitsFromBits(AlignmentInBits).getAsAlign();
     AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
     AI->setAlignment(AlignmentInBytes);
     if (BuiltinID != Builtin::BI__builtin_alloca_with_align_uninitialized)
       initializeAlloca(*this, AI, Size, AlignmentInBytes);
+    if (getASTAllocaAddressSpace() != LangAS::Default)
+      return RValue::get(Builder.CreateAddrSpaceCast(AI, CGM.Int8PtrTy));
+    else
       return RValue::get(AI);
   }
 
   case Builtin::BIbzero:
   case Builtin::BI__builtin_bzero: {
     Address Dest = EmitPointerWithAlignment(E->getArg(0));
     Value *SizeVal = EmitScalarExpr(E->getArg(1));
     EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(0)->getType(),
                         E->getArg(0)->getExprLoc(), FD, 0);

clang/test/CodeGen/dynamic-alloca-with-address-space.c

+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -emit-llvm %s -o - | FileCheck %s
+
+void allocas(unsigned long n) {
+    void *a = __builtin_alloca(n);
+    void *uninitialized_a = __builtin_alloca_uninitialized(n);
+    void *aligned_a = __builtin_alloca_with_align(n, 8);
+    void *aligned_uninitialized_a = __builtin_alloca_with_align_uninitialized(n, 8);
+}
+
+// CHECK: @allocas(
+// CHECK: store i64 %n, ptr %n.addr.ascast, align 8
+// CHECK: %0 = load i64, ptr %n.addr.ascast, align 8
+// CHECK: %1 = alloca i8, i64 %0, align 8, addrspace(5)
+// CHECK: %2 = addrspacecast ptr addrspace(5) %1 to ptr
+// CHECK: store ptr %2, ptr %a.ascast, align 8
+// CHECK: %3 = load i64, ptr %n.addr.ascast, align 8
+// CHECK: %4 = alloca i8, i64 %3, align 8, addrspace(5)
+// CHECK: %5 = addrspacecast ptr addrspace(5) %4 to ptr
+// CHECK: store ptr %5, ptr %uninitialized_a.ascast, align 8
+// CHECK: %6 = load i64, ptr %n.addr.ascast, align 8
+// CHECK: %7 = alloca i8, i64 %6, align 1, addrspace(5)
+// CHECK: %8 = addrspacecast ptr addrspace(5) %7 to ptr
+// CHECK: store ptr %8, ptr %aligned_a.ascast, align 8
+// CHECK: %9 = load i64, ptr %n.addr.ascast, align 8
+// CHECK: %10 = alloca i8, i64 %9, align 1, addrspace(5)
+// CHECK: %11 = addrspacecast ptr addrspace(5) %10 to ptr
+// CHECK: store ptr %11, ptr %aligned_uninitialized_a.ascast, align 8
+// CHECK: ret void