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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson marked 2 inline comments as done.
arichardson added a comment.

In https://reviews.llvm.org/D47541#1158529, @bjope wrote:

> This seems to work quite nice for me now! Nice!
>
> I added a few inline nit comments. Apart from that it looks good to me.
>  Although, I don't know much about the bitcode reader/writer. Anyone else who can help out reviewing that part?
>
> PS. I still have a few worries about how well this plays together with instrprof, we have had some hacks in that area earlier. Methods like appendToGlobalArray (in ModuleUtils.cpp) still uses PointerType::getUnqual(FnTy) to strip off the addrspace from function pointers (and afaik the size of a pointer can be different in different adress spaces).
>  Although, I don't think we can solve all problems right away. This patch at least gives us the power to set addrspace on functions. And that should help out if I for example want to reproduce instrprof problems on trunk (using an "experimental target" with non-zero program addrspace). So I think we can leave that for later.


In our fork I've deleted all uses of PointerType::getUnqual()  from clang, and removed those in LLVM that have caused issues. I'm hoping to slowly upstream those patches and eventually remove PointerType::getUnqual().
I agree that those problems should be addressed by a later patch.



================
Comment at: lib/AsmParser/LLParser.cpp:1521
 ///   := 'addrspace' '(' uint32 ')'
 bool LLParser::ParseOptionalAddrSpace(unsigned &AddrSpace) {
   AddrSpace = 0;
----------------
bjope wrote:
> Fwiw, I created a ParseOptionalProgramAddrSpace method when adapting this to our out-of-tree target. And then I use that method when parsing the optional addrspace string when we know that it is a program address space (in define/declare/call/invoke).
> One still needs to be explicit with addrspace when dealing with function pointers (in casts etc), but that is how we have been doing it in the past. And the use of datalayout ofcourse means that I need to make sure the datalayout is specified early in the ll-file.
> Anyway, the above gave me the possibility to override the default value 0 (e.g. by using M->getDataLayout().getProgramAddressSpace()) for our target. We will at least use that as a temporary solution until we have adapted all our frontends (and test cases) to use the new syntax.
> 
> 
Thanks, that's a great suggestion! It should allow us to keep our existing tests without adding addrspace(200) to every call instr. I'll update the patch to use that instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D47541