Page MenuHomePhabricator

[CVP] Fold return values if possible
ClosedPublic

Authored by reames on Nov 2 2015, 5:38 PM.

Details

Summary

In my previous change to CVP (251606), I made CVP much more aggressive about trying to constant fold comparisons. This patch is a reversal in direction. Rather than being agressive about every compare, we restore the non-block local restriction for most, and then try hard for compares feeding returns.

The motivation for this is two fold:

  1. The more I thought about it, the less comfortable I got with the possible compile time impact of the other approach. There have been no reported issues, but after talking to a couple of folks, I've come to the conclusion the time probably isn't justified.
  2. It turns out we need to know the context to leverage the full power of LVI. In particular, asking about something at the end of it's block (the use of a compare in a return) will frequently get more precise results than something in the middle of a block. This is an implementation detail, but it's also hard to get around since mid-block queries have to reason about possible throwing instructions and don't get to use most of LVI's block focused infrastructure. This will become particular important when combined with http://reviews.llvm.org/D14263.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 39011.Nov 2 2015, 5:38 PM
reames retitled this revision from to [CVP] Fold return values if possible.
reames updated this object.
reames added reviewers: sanjoy, hfinkel, nlewycky.
reames added a subscriber: llvm-commits.
nlewycky added inline comments.Nov 2 2015, 6:04 PM
lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
52 ↗(On Diff #39011)

"Constant*" -> "Constant *"

198 ↗(On Diff #39011)

chose -> choose (present tense)

200 ↗(On Diff #39011)

"it's not it's primary purposes" -> "it's not its primary purpose". (it's -> its and purposes -> purpose)

333 ↗(On Diff #39011)

Constant* -> Constant *

337 ↗(On Diff #39011)

algorith -> algorithm

339–340 ↗(On Diff #39011)

What about a single SelectInst?

351–354 ↗(On Diff #39011)

Remove the else. See http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

(Yes I know you just copied it from code above.)

test/Transforms/CorrelatedValuePropagation/select.ll
74 ↗(On Diff #39011)

Cool!

reames updated this revision to Diff 39023.Nov 2 2015, 6:12 PM

Address Nick's review.

reames added inline comments.Nov 2 2015, 6:13 PM
lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
339–340 ↗(On Diff #39023)

Do you have a particular pattern you'd want to handle? My preference would be a) a follow on change and b) not doing this until I can get this pushed down into LVI. (That's on my list, just need to clear some other LVI/CVP changes out first.)

nlewycky accepted this revision.Nov 2 2015, 10:28 PM
nlewycky edited edge metadata.

Forgot to LGTM

This revision is now accepted and ready to land.Nov 2 2015, 10:28 PM
This revision was automatically updated to reflect the committed changes.