Page MenuHomePhabricator

[IPSCCP] Use constant range information for comparisons of parameters.
ClosedPublic

Authored by fhahn on Feb 26 2018, 6:18 AM.

Details

Summary

For comparisons with parameters, we can use the ParamState lattice
elements which also provide constant range information. This improves
the code for PR33253 further and gets us closer to use
ValueLatticeElement for all values.

Also, as we are using the range information in the solver directly, we
do not need tryToReplaceWithConstantRange afterwards anymore.

Diff Detail

Repository
rL LLVM

Event Timeline

mssimpso added inline comments.Mar 22 2018, 8:19 AM
test/Transforms/SCCP/ip-constant-ranges.ll
5 ↗(On Diff #135892)

Hi Florian,

I'm not understanding why we end up with undef here. Can you help me out?

mssimpso added inline comments.Mar 22 2018, 8:27 AM
test/Transforms/SCCP/ip-constant-ranges.ll
5 ↗(On Diff #135892)

Ah, it's because the function becomes dead after the simplification in @caller1. Right?

fhahn updated this revision to Diff 139461.Mar 22 2018, 10:21 AM
fhahn edited the summary of this revision. (Show Details)

Removed tryToReplaceWithConstantRange. It's not needed anymore, as we use the constant range info directly in the solver.

fhahn added inline comments.Mar 22 2018, 10:22 AM
test/Transforms/SCCP/ip-constant-ranges.ll
5 ↗(On Diff #135892)

Yep. And the selects are removed, because now the range info is used directly in the solver and is used to simplify the function further.

mssimpso accepted this revision.Mar 22 2018, 1:33 PM

LGTM.

This revision is now accepted and ready to land.Mar 22 2018, 1:33 PM
fhahn updated this revision to Diff 139572.Mar 23 2018, 3:50 AM

Slightly restructured to only call getValueState when we failed to find a value in ParamState.

This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Apr 5 2018, 10:25 AM

I had to revert this patch a while ago in rL328312 because it did a few things wrong. Most issues are fixed now, there is still one failure I need to track down. I will share the updated patches soon.

fhahn added a comment.Jul 3 2018, 3:13 AM

I've finally had some time to come back to this change (sorry for the long delay!) and committed an updated version of this patch as rL336098.

The change to the original patch is that we have to add values to the worklist if merging in an argument changes the ParamState for it, but there has been no change in ValueState (e.g. ValueState is overdefined, but in ParamState we have a value range that changed). It would be great if could have a quick look at the committed change as a sanity check I did not miss anything.