Page MenuHomePhabricator

[IR] Convert null-pointer-is-valid into an enum attribute
ClosedPublic

Authored by nikic on Apr 25 2020, 7:00 AM.

Details

Summary

The "null-pointer-is-valid" attribute needs to be checked by many pointer-related combines. To make the check more efficient, convert it from a string into an enum attribute.

While D78859 already makes the check for the attribute cheaper, this is another 0.4% improvement.

Diff Detail

Event Timeline

nikic created this revision.Apr 25 2020, 7:00 AM

Just wondering if this allso enables

attribute((null_pointer_is_valid)) in Clang?

nikic added a comment.Apr 25 2020, 8:10 AM

@xbolva00 This should not change any front-end behavior. I believe in clang this is controlled by -fno-delete-null-pointer-checks.

nikic updated this revision to Diff 260105.Apr 25 2020, 8:35 AM

Update tests

Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
arsenm added a subscriber: arsenm.Apr 25 2020, 9:20 AM

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

Also would be address space specific

FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed

Is this change planned more for the near term or the long term? I'm not really familiar with ongoing addrspace related work.

nikic added a comment.Apr 26 2020, 1:04 AM

@arsenm If the null-pointer-is-valid attribute is moved into the data layout, I'm wondering how Clang's -fno-delete-null-pointer-checks option would work or what it would be replaced with. In Rust it is possible to define a custom target, which also defines a custom data-layout, though I think that also needs to be compatible with the base target. I couldn't find any information on how to use a custom data layout in Clang. As such, I suspect that to preserve existing functionality we'd need support both for specifying this per-addrspace in the DL and as a function attribute.

ftynse resigned from this revision.Apr 27 2020, 4:40 AM
nikic edited reviewers, added: manojgupta, efriedma; removed: sstefan1, ftynse.Apr 28 2020, 11:53 AM
nikic edited the summary of this revision. (Show Details)Apr 28 2020, 11:54 AM

@nikic Thanks for the work.

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.

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?

@nikic Thanks for the work.

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.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1304–1305

I guess this change can go in as NFC simplification right away.

nikic updated this revision to Diff 261279.Apr 30 2020, 10:22 AM
nikic marked an inline comment as done.

Rebase over committed NFC changes.

nikic updated this revision to Diff 261375.Apr 30 2020, 3:11 PM

Fix rebase mistake in test

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

@nikic Thanks for the work.

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

jdoerfert accepted this revision.May 14 2020, 9:21 PM

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

Given that we lifted the limits on enum attributes I don't think this cost is high. Since we do not have a scheduled alternative, I think this should land.

This revision is now accepted and ready to land.May 14 2020, 9:21 PM
This revision was automatically updated to reflect the committed changes.