[PATCH] D78862: [IR] Convert null-pointer-is-valid into an enum attribute

Fri May 8 11:14:50 PDT 2020

arsenm added a comment.

In D78862#2012560 <https://reviews.llvm.org/D78862#2012560>, @jdoerfert wrote:

> I think this is an improvement over the status quo and it looks fine to me.
>
> @arsenm I agree that we should tie this to the data layout (or at least should try) but I guess there are open questions to answer and code to write.
>  I propose to accept this and work on the DL patch after. WDYT?

Seems ok, but it's still burning an enum value which I guess isn't super important. With the datalayout property, we might really want the inverse attribute

> 
> 
> In D78862#2008831 <https://reviews.llvm.org/D78862#2008831>, @manojgupta wrote:
> 
>> @nikic Thanks for the work.
>>
>> In D78862#2003684 <https://reviews.llvm.org/D78862#2003684>, @arsenm wrote:
>>
>> > FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed
>>
>>
>> @arsenm  Is there any work planned on moving to data layout? Moving to data layout may affect cross TU inlining e.g. LTO where 1 TU is compiled with `-fno-delete-null-pointer-checks` and other TU is not. There might be other potential impact that we might not know yet.
> 
> 
> If we link modules with mismatching data layouts we can/should deal with this by utilizing more address spaces. That is, change the address space in one module to a fresh one to keep the properties alive. There need to be rules for this and infrastructure but something similar might be needed for heterogeneous IR modules soon. Different story though. We can also combine the attribute and the data layout if necessary, though I'm not a fan.

This sounds really problematic and would require way more knowledge of target address spaces. I don't think this will work

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78862