[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:
> bjope wrote:
> > arichardson wrote:
> > > dylanmckay wrote:
> > > > bjope wrote:
> > > > > 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.)
> > > > > 
> > > > > 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 suspect @arsenm is suggesting that the address space be copied from the `LLParserPerFunctionState* PFS` argument this function has.
> > > > 
> > > > For example, replace `M->getDataLayout().getProgramAddressSpace()` with `PFS->getFunction()->getFunctionType()->getPointerAddressSpace()`
> > > > 
> > > > - [`PerFunctionState::getFunction` documentation](https://ldhldh.myds.me:10081/docs/llvm700/classllvm_1_1_l_l_parser_1_1_per_function_state.html#a9b14016c1937d715c8f305742547764e)
> > > > 
> > > > This seems like a better alternative, as it means that if a function did opt to use a different address space from the program address space in the data layout, the blockaddresses within it will use that same address space rather than assuming the one from the datalayout.
> > > > 
> > > > This also makes an assumption about the address space of the target function, although I feel the case for that assumption is stronger than the current one of "assume address space from datalayout" as the address space from the parent function could be considered "closer to the source". Indeed, we directly lookup the actual address space for this block address in this branch as this is the case for when the actual function has not been defined yet and so the address space information is not available.
> > > > 
> > > > Please make this replacement and then the patch should be good to go. 
> > > Thanks, will do that and add a test case.
> > Well, taking the address space from the //wrong// function does not help for "You should be able to mix functions with different address spaces in the same module.".
> > 
> > If heading in that direction then please add some nifty code comment explaining what is going on. When using the datalayout from the module it is easy to understand that we aren't using any function specific information, but if you use the function pointer from the wrong function to derive the address space it might fool someone that it either is the correct function that is being used, or that it is an "unintended bug" rather than a "hacky workaround".
> block address is always in the context of a function, there's no wrong function to choose. You just use the address space from the parent function
Sure, so then we agree that we don't mix address spaces for program (there can be only one and taking it from any function would be ok).

Still, there are at least some lit test cases (for example the AVR/brind.ll test modified in this patch) that have global variables involving blockaddress with forward references to functions not yet defined. In such cases there is no parent function, right? We only got the function referenced in the blockaddress argument, and its definition might not have been parsed yet. Or maybe I've simply misunderstood when we end up in this part of the code (if all the test cases pass I guess things are in order).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48803