Details

Reviewers

jdoerfert
manojgupta
efriedma
sstefan1

Commits

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

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

Repository: rG LLVM Github Monorepo

Event Timeline

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

Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2020, 7:00 AM

Herald added subscribers: llvm-commits, dexonsmith, steven_wu, hiraditya. · View Herald Transcript

Just wondering if this allso enables

attribute((null_pointer_is_valid)) in Clang?

Harbormaster failed remote builds in B54677: Diff 260092!Apr 25 2020, 7:56 AM

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

Update tests

Herald added a reviewer: jdoerfert. · View Herald TranscriptApr 25 2020, 8:35 AM

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

Herald added subscribers: cfe-commits, Kayjukh, frgossen and 15 others. · View Herald Transcript

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

In D78862#2003684, @arsenm wrote:

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

Harbormaster failed remote builds in B54685: Diff 260105!Apr 25 2020, 9:32 AM

In D78862#2003684, @arsenm wrote:

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.

@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

Herald added a reviewer: sstefan1. · View Herald TranscriptApr 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.

In 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.

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, @manojgupta wrote:

@nikic Thanks for the work.

In 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.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1304–1305	I guess this change can go in as NFC simplification right away.

Rebase over committed NFC changes.

Harbormaster failed remote builds in B55332: Diff 261279!Apr 30 2020, 11:15 AM

Fix rebase mistake in test

Harbormaster failed remote builds in B55381: Diff 261375!Apr 30 2020, 4:07 PM

Ping

Herald added a subscriber: stephenneuendorffer. · View Herald TranscriptMay 8 2020, 1:15 AM

In 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, @manojgupta wrote:

@nikic Thanks for the work.

In 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

In D78862#2027254, @arsenm wrote:

In 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

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

Herald added subscribers: jurahul, kuter. · View Herald TranscriptMay 14 2020, 9:21 PM

Closed by commit rGf89f7da999f3: [IR] Convert null-pointer-is-valid into an enum attribute (authored by nikic). · Explain WhyMay 15 2020, 10:52 AM

This revision was automatically updated to reflect the committed changes.

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 264285

clang/lib/CodeGen/CGCall.cpp

clang/test/CodeGen/delete-null-pointer-checks.c

llvm/docs/LangRef.rst

llvm/include/llvm/Bitcode/LLVMBitCodes.h

llvm/include/llvm/IR/Attributes.td

llvm/include/llvm/IR/AutoUpgrade.h

llvm/lib/AsmParser/LLLexer.cpp

llvm/lib/AsmParser/LLParser.cpp

llvm/lib/AsmParser/LLToken.h

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

llvm/lib/IR/Attributes.cpp

llvm/lib/IR/AutoUpgrade.cpp

llvm/lib/IR/Function.cpp

llvm/lib/IR/Verifier.cpp

llvm/lib/Transforms/Utils/CodeExtractor.cpp

llvm/test/Analysis/MemorySSA/cyclicphi.ll

llvm/test/Analysis/ValueTracking/assume.ll

llvm/test/Bitcode/attributes.ll

llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll

llvm/test/Transforms/Attributor/align.ll

llvm/test/Transforms/Attributor/nocapture-1.ll

llvm/test/Transforms/Attributor/nonnull.ll

llvm/test/Transforms/Attributor/norecurse.ll

llvm/test/Transforms/Attributor/undefined_behavior.ll

llvm/test/Transforms/CorrelatedValuePropagation/non-null.ll

llvm/test/Transforms/FunctionAttrs/nocapture.ll

llvm/test/Transforms/FunctionAttrs/nonnull.ll

llvm/test/Transforms/GVN/PRE/2018-06-08-pre-load-dbgloc-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/MallocSROA-section-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/heap-sra-1-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/heap-sra-1.ll

llvm/test/Transforms/GlobalOpt/heap-sra-2-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/heap-sra-2.ll

llvm/test/Transforms/GlobalOpt/heap-sra-3-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/heap-sra-3.ll

llvm/test/Transforms/GlobalOpt/heap-sra-4-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/heap-sra-4.ll

llvm/test/Transforms/GlobalOpt/heap-sra-phi-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/heap-sra-phi.ll

llvm/test/Transforms/GlobalOpt/load-store-global-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/malloc-promote-1-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/malloc-promote-1.ll

llvm/test/Transforms/GlobalOpt/malloc-promote-2-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/malloc-promote-2.ll

llvm/test/Transforms/GlobalOpt/storepointer-compare-no-null-opt.ll

llvm/test/Transforms/GlobalOpt/storepointer-no-null-opt.ll

llvm/test/Transforms/IPConstantProp/PR26044.ll

llvm/test/Transforms/Inline/attributes.ll

llvm/test/Transforms/InstCombine/atomic.ll

llvm/test/Transforms/InstCombine/invariant.group.ll

llvm/test/Transforms/InstCombine/invoke.ll

llvm/test/Transforms/InstCombine/lifetime-no-null-opt.ll

llvm/test/Transforms/InstCombine/load.ll

llvm/test/Transforms/InstCombine/mem-deref-bytes.ll

llvm/test/Transforms/InstCombine/memchr.ll

llvm/test/Transforms/InstCombine/memcpy-addrspace.ll

llvm/test/Transforms/InstCombine/memcpy-from-global.ll

llvm/test/Transforms/InstCombine/memrchr.ll

llvm/test/Transforms/InstCombine/select.ll

llvm/test/Transforms/InstCombine/store.ll

llvm/test/Transforms/InstCombine/strchr-1.ll

llvm/test/Transforms/InstCombine/strcpy_chk-64.ll

llvm/test/Transforms/InstCombine/strlen-1.ll

llvm/test/Transforms/InstCombine/strncat-2.ll

llvm/test/Transforms/InstCombine/strncmp-1.ll

llvm/test/Transforms/InstCombine/strrchr-1.ll

llvm/test/Transforms/InstCombine/strstr-1.ll

llvm/test/Transforms/InstCombine/wcslen-1.ll

llvm/test/Transforms/InstSimplify/compare.ll

llvm/test/Transforms/LoopIdiom/pr28196.ll

llvm/test/Transforms/LoopVersioning/lcssa.ll

llvm/test/Transforms/SimplifyCFG/UnreachableEliminate.ll

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

Revision Contents
Changeset List