Page MenuHomePhabricator

[CVP] Use LVI to constant fold deopt operands
ClosedPublic

Authored by reames on Dec 13 2018, 2:51 PM.

Details

Summary

Deopt operands are generally intended to record information about a site in code with minimal perturbation of the surrounding code. Idiomatically, they also tend to appear down rare paths. Putting these together, we have an obvious case for extending CVP w/deopt operand constant folding. Arguably, we should be doing this for all operands on all instructions, but that's definitely a much larger and risky change.

A few items to note for the review:

  • The const_cast is definitely ugly, but the API for operand bundles seemed to require it. Is there a better way to structure this?
  • As noted w/a TODO, I initially tried to use a context instruction here, but discovered that the result returned by LVI was actually wrong. It seemed return something true *after* the context instruction, not before it. For an instruction like a guard or assume, that difference is quite material!

Diff Detail

Event Timeline

reames created this revision.Dec 13 2018, 2:51 PM
anna added inline comments.Dec 21 2018, 10:33 AM
lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
471

Nit: reasonably.

485

From the code, getConstant calls getValueInBlock which returns the value at the end of the BasicBlock. The BB passed in here is the callsite's parent. We want the value which is true at the beginning of the CS.getParent() right?

reames planned changes to this revision.Dec 21 2018, 10:57 AM
reames marked an inline comment as done.
reames added inline comments.
lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
485

Eek, good catch. Will have to readjust how I approach this.

reames updated this revision to Diff 181718.Jan 14 2019, 9:30 PM

Switch batch to using the context instruction, and fix the bug it exposed. As far as I can tell, the bug was invisible for all existing code since the context instruction had to be the guard itself.

anna accepted this revision.Jan 18 2019, 3:59 PM

LGTM!

This revision is now accepted and ready to land.Jan 18 2019, 3:59 PM
This revision was automatically updated to reflect the committed changes.