This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Extend range metadata to call/invoke
ClosedPublic

Authored by jingyue on Jun 17 2014, 5:38 PM.

Details

Summary

With this patch, range metadata can be added to call/invoke including
IntrinsicInst. Previously, it could only be added to load.

Rename computeKnownBitsLoad to computeKnownBitsFromRangeMetadata because
range metadata is not only used by load.

Update the language reference to reflect this change.

Diff Detail

Event Timeline

jingyue updated this revision to Diff 10522.Jun 17 2014, 5:38 PM
jingyue retitled this revision from to [ValueTracking] Extend range metadata to call/invoke.
jingyue updated this object.
jingyue edited the test plan for this revision. (Show Details)
jingyue added reviewers: eliben, hfinkel, meheff, nlewycky, reames.
jingyue added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.Jun 18 2014, 1:02 AM

LGTM, thanks!

lib/Analysis/ValueTracking.cpp
201

I don't like the extra spaces here around the * operator.

740

We don't need {} around a single-statement if body.

eliben added inline comments.Jun 18 2014, 6:10 AM
lib/Analysis/ValueTracking.cpp
741

Does this mean that for known intrinsics, even if their invocations have range metadata it will be overridden by fixed limits? I wonder if this is the right thing to do. Maybe for a specific intrinsic call we have "insider information" knowing that its return value is even more limited than usual - it would be nice to be able to express this with range metadata, no?

hfinkel added inline comments.Jun 18 2014, 7:55 AM
lib/Analysis/ValueTracking.cpp
741

I agree; it looks like you should change the below from KnownZero = to KnownZero |= (or something like that).

reames edited edge metadata.Jun 18 2014, 10:20 AM

LGTM.

docs/LangRef.rst
2754

The wording here is strange:
"return value of a function call is in"

What does that mean? Possibly:
"value returned by the called function at this call site"

jingyue updated this revision to Diff 10599.Jun 18 2014, 5:21 PM
jingyue edited edge metadata.

Thanks for reviewing! Addressed all comments.

eliben edited edge metadata.Jun 18 2014, 8:26 PM

Could you add a test to see that this OR-ing of known bits really works; for example by attaching some range metadata to a ctlz intrinsic call?

jingyue updated this revision to Diff 10604.Jun 18 2014, 10:48 PM
jingyue edited edge metadata.

Good point! Added two tests that verify the intersection works as expected.

jingyue updated this revision to Diff 10605.Jun 18 2014, 10:56 PM

Moved two tests to add2.ll because they are related to the add-to-or
optimization.

LGTM, thanks!

eliben accepted this revision.Jun 19 2014, 6:07 AM
eliben edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 19 2014, 6:07 AM
jingyue updated this revision to Diff 10644.Jun 19 2014, 9:57 AM
jingyue edited edge metadata.

Rebased against master

jingyue closed this revision.Jun 19 2014, 9:58 AM

Committed in r211281. Will watch the bot.