[PATCH] D48803: Place the BlockAddress type in the program address space

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:3394
+            nullptr, "", nullptr, GlobalValue::NotThreadLocal,
+            M->getDataLayout().getProgramAddressSpace());
+      }
----------------
arsenm wrote:
> Why wouldn't this come from the parent function? You should be able to mix functions with different address spaces in the same module
(Maybe @arichardson got a different reason, but sharing my point-of-view here anyway.)

While it's possible to annotate calls and functions definitions with non-zero program address spaces, I think one need to be consistent. I don't think we really support multiple program address spaces (is there an actual use case for supporting that?).

I'm also not exactly sure what you mean by "parent function". The addrspce in the resulting pointer type need to match the addrspace of the function referenced in the first argument of the blockaddress. And that function has not been defined yet, since we are inside the "!F" clause.

I guess we have to trust the datalayout if the function hasn't been defined yet (or use some kind of forward ref and backtrack to fill in addrspace to get the correct type later). I wonder if we'd get some kind of type error later if we assume that datalayout is correct here, and we find a different addrspace when finding the function definition later?

(We also got the usual problem that if datalayout is set by a datalayout definition that comes later in the ll file we haven't parsed the datalayout yet. But if I remember correclty that is a general problem also for the function definitions etc.)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D48803/new/

https://reviews.llvm.org/D48803