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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

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.



================
Comment at: lib/AsmParser/LLParser.cpp:1333
+    return Val;
+  // For calls we also accept variables in the program address space
+  Type* SuggestedTy = Ty;
----------------
nit: End code comment with a punctuation.


================
Comment at: lib/AsmParser/LLParser.cpp:1521
 ///   := 'addrspace' '(' uint32 ')'
 bool LLParser::ParseOptionalAddrSpace(unsigned &AddrSpace) {
   AddrSpace = 0;
----------------
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.




================
Comment at: lib/IR/Function.cpp:224
+  // If AS == -1 and we are passed a valid module pointer we place the function
+  // in the program address space and otherwise we default to AS0
+  if (AddrSpace == static_cast<unsigned>(-1))
----------------
nit: punctuation


Repository:
  rL LLVM

https://reviews.llvm.org/D47541