This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Transform !range metadata to !nonnull when combining loads
ClosedPublic

Authored by arielb1 on Oct 3 2016, 4:38 PM.

Details

Summary

When combining an integer load with !range metadata that does not include 0 to a pointer load, make sure emit !nonnull metadata on the newly-created pointer load. This prevents the !nonnull metadata from being dropped during a ptrtoint/inttoptr pair.

This fixes PR30597

Diff Detail

Repository
rL LLVM

Event Timeline

arielb1 updated this revision to Diff 73371.Oct 3 2016, 4:38 PM
arielb1 retitled this revision from to [InstCombine] Transform !range metadata to !nonnull when combining loads.
arielb1 updated this object.
arielb1 added a reviewer: chandlerc.
arielb1 set the repository for this revision to rL LLVM.
arielb1 added a subscriber: llvm-commits.
hfinkel added a subscriber: hfinkel.Oct 3 2016, 4:48 PM
hfinkel added inline comments.
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
386

What is the:

N->getNumOperands() == 2

supposed to be checking? The range metadata can have any even number of operands; and we already have an assert for this in llvm::getConstantRangeFromMetadata.

387

Indentation looks odd here. Tabs?

test/Transforms/InstCombine/PR30597.ll
24

This is a nice test case, and I'd keep that, but can you also add a more-direct test case involving the range metadata directly?

arielb1 updated this revision to Diff 73377.Oct 3 2016, 5:21 PM
arielb1 removed rL LLVM as the repository for this revision.

Address nits and add test.

arielb1 updated this revision to Diff 73383.Oct 3 2016, 5:27 PM
arielb1 set the repository for this revision to rL LLVM.

Fix submission errors.

Please upload your patches with full context; see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for instructions.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
1290

Don't commit this white-space change.

test/Transforms/InstCombine/PR30597-2.ll
1

I don't see any need to have both of these tests in different files. Please put them in the same file.

test/Transforms/InstCombine/PR30597.ll
1

I understand why you want to run -simplifycfg here to cleanup the output of instcombine for the check, but we generally prefer to use the minimal set of passes. Please just run instcombine and check that the branch condition in the output is the appropriate constant.

majnemer added inline comments.
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
387

Generally, we just say unsigned.

test/Transforms/InstCombine/PR30597-2.ll
7

You should probably pattern match whatever %1 happens to be. We should also match the return.

7

What does !0 correspond to? Please match it.

test/Transforms/InstCombine/PR30597.ll
1

I'd go even further and suggest that the test be amended to return an i1.

arielb1 updated this revision to Diff 73450.Oct 4 2016, 4:05 AM

address review comments.

arielb1 updated this revision to Diff 73451.Oct 4 2016, 4:06 AM

unsigned int -> unsigned.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
386

Cargo cult copypasta. Will remove

387

Sure.

majnemer accepted this revision.Oct 10 2016, 11:19 AM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Oct 10 2016, 11:19 AM
majnemer closed this revision.Oct 10 2016, 6:11 PM

Committing to https://llvm.org/svn/llvm-project/llvm/trunk ...
A test/Transforms/InstCombine/PR30597.ll
M lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
Committed r283836