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

Thu Apr 30 06:12:54 PDT 2020

jdoerfert added a comment.

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?

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.

================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1321
-        I == Attribute::NoSync)
-      continue;
     if (uint64_t A = (Val & getRawAttributeMask(I))) {
----------------
I guess this change can go in as NFC simplification right away.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78862