[PATCH] D47541: Allow creating llvm::Function in non-zero address spaces

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/AsmParser/LLParser.cpp:1272
+    Type *TyInProgAS = cast<PointerType>(Ty)->getElementType()->getPointerTo(
+        M->getDataLayout().getProgramAddressSpace());
+    SuggestedTy = TyInProgAS;
----------------
bjope wrote:
> This only works if we already have parsed the DataLayout string.
> 
> If we need to rely on having parsed the DataLayout string (or even relying on that there is one in the LL file), why should we then need to decorate functions with addrspace attributes instead of simply using the ProgramAddressSpace from the DataLayout?
> 
> Wasn't the usage of addrspace attributes on functions a way to avoid the dependecy to a potentially not yet parsed DataLayout inside LLParser (see RFC discussion here http://lists.llvm.org/pipermail/llvm-dev/2017-July/115388.html).
> 
> I think there are at least three alternatives:
> 
> 1) Make the DataLayout string with 'P' mandatory (early in LL file) when using non-zero program addrspace. Then maybe the simplest thing is to use M->getDataLayout().getProgramAddressSpace() in ParseFunctionHeader (line 5064) and skip the explicit addrspace attributes on function define/declare.
> 
> 2) Skip verifying (during parsing) that the addrspace matches for forward declaration of a function (at least when originating from a call). The define/declare then needs to replace/fixup the Function to use the correct address space. Although I'm not sure exactly how to do this.
> 
> 3) Make it possible to indicate addrspace of a function in the call statement. I guess we would need to come up with a new syntax for that.
> 
> I'm a little worried that the need to add addrspace attributes on function declarations will make it a bit more cumbersome to write test cases. That is ofcourse not a major problem. Alternative 3 will make it even more complicated. Alternative 1 is probably the simplest, and even if it does not address everything from the RFC, at least it will move us forward a little bit.
> 
We can add at the following as a fourth altenative (as a variant of 1):
4) Make the DataLayout string with 'P' mandatory (early in LL file or on the command line) when using non-zero program addrspace (at least until we find a better solution). But also require that addrspace is explicitly set on function definition/declarations. I think that makes alternative 4 what we currently have in this patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D47541