This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by AlexVlx on Jul 28 2023, 7:03 AM.

Details

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.

Diff Detail

Event Timeline

AlexVlx created this revision.Jul 28 2023, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2023, 7:03 AM
Herald added a subscriber: arichardson. · View Herald Transcript
AlexVlx requested review of this revision.Jul 28 2023, 7:03 AM

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 added a subscriber: arsenm.Jul 28 2023, 9:08 AM

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 added inline comments.Jul 28 2023, 9:15 AM
clang/lib/CodeGen/CGBuiltin.cpp
3550

No return after else

3572

No return after else

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 updated this revision to Diff 545679.Jul 31 2023, 8:25 AM
AlexVlx added a reviewer: arsenm.

Incorporated review feedback. Updated test.

AlexVlx marked 2 inline comments as done.Jul 31 2023, 8:25 AM
arsenm added inline comments.Jul 31 2023, 9:07 AM
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 added inline comments.Jul 31 2023, 9:09 AM
clang/test/CodeGen/dynamic-alloca-with-address-space.c
2

This is not valid 1.2 code, for 2.0 sure.

arsenm added inline comments.Jul 31 2023, 9:10 AM
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)

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.

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 updated this revision to Diff 545860.Jul 31 2023, 4:28 PM

Address review comments.

AlexVlx marked 2 inline comments as done.Jul 31 2023, 4:28 PM
rjmccall added inline comments.Jul 31 2023, 4:33 PM
clang/lib/CodeGen/CGBuiltin.cpp
3542

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

AlexVlx updated this revision to Diff 545882.Jul 31 2023, 5:46 PM

Use pointee's address space for the check.

AlexVlx marked an inline comment as done.Jul 31 2023, 5:47 PM
AlexVlx added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
3542

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

rjmccall accepted this revision.Aug 1 2023, 11:03 AM

No problem, LGTM.

This revision is now accepted and ready to land.Aug 1 2023, 11:03 AM
This revision was landed with ongoing or failed builds.Aug 1 2023, 1:55 PM
This revision was automatically updated to reflect the committed changes.
AlexVlx marked an inline comment as done.