[PATCH] D31042: Allow DataLayout to specify addrspace for allocas.

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D31042#711913, @arsenm wrote:

> In https://reviews.llvm.org/D31042#711863, @rjmccall wrote:
>
> > I don't think the assumptions in Clang that the Clang AST address space 0 equals LLVM's address space 0 are nearly as pervasive as you think.  At any rate, since you require stack allocations to be in a different LLVM address space from the generic address space, but (per C) taking the address of a local variable yields a pointer in the generic address space, you're going to have to teach Clang about these differences between address spaces anyway, because it'll have to insert address-space conversions in a number of places.
>
>
> Yes, I expect there will be many addrspacecasts inserted from the alloca, which then the InferAddressSpaces pass would hopefully optimize away.


Okay.  So, to summarize:

In amdgpu OpenCL, at source level, sizeof(void __private *) == sizeof(void {__global,__local,__constant} *) == 8.  However, at an implementation level, stack addresses are actually 32-bit, and you want that to be modeled correctly in the IR.  Therefore, allocas need to return a value in a 32-bit address space, which the frontend will immediately widen to a 64-bit address space in order to match the rules for __private.  In order to maintain performance, you have a pass that recognizes when an address value is the result of that kind of widening and re-narrows it to the 32-bit stack address space.

Because LLVM currently makes some unfortunate assumptions about address space 0, this stack address space cannot be address space 0.  These assumptions include:
(1) As far as debug info is concerned, the size of a pointer is the size of a pointer in address space 0.  (Is this assumption why sizeof(void __private *) == 8 despite __private being clearly intended in the OpenCL spec to be the address space of stack pointers?)
(2) Functions are currently always defined in address space 0.  A function pointer in your implementation must be in a 64-bit address space.  (My understanding was that OpenCL generally forbade function pointers, so I'm not sure if this is actually important?)
(3) Some optimizations assume that 0 is an invalid address in address space 0, but in fact it's a valid stack address.

It's entirely possible that we should, indeed, change all of these assumptions.  For the time being, you feel that's not a reasonable fight, and so you would like to allow IR to use an explicitly non-0 address space for the stack.

I think I'm prepared to accept your argument.

>>> I don't think adding this DataLayout argument is very invasive, and was less work than I anticipated
>> 
>> That's because you aren't changing any of the frontends, most of which are not in-tree.  I understand that LLVM does not promise to keep the C++ API stable, but there's surely *some* responsibility to not break every frontend for features they don't care about.  At the very least, yes, you should be picking up this value from the Module instead of requiring it to be passed in.
> 
> So your objection is just to the API change? I can change that if you insist. The actual mechanical adding of the argument isn't the source of the work. As someone who has maintained out of tree uses, this class of change is the least irritating. Most of the clang IR gen work is in updating the downstream users which use hardcoded pointer types, which users not using this don't need to care about it.

Yes, I would like you to avoid changing the IRBuilder API if you can.  I can see why you need to change the AllocaInst constructors, though.

Do you actually need this to be printed/parsed in .ll files?  It's globally consistent across all alloca instructions, and the data layout is always parsed before anything else in the file.  It's not the end of the world, just seems odd to imply that this is actually something that can be different on different instructions.


https://reviews.llvm.org/D31042