Page MenuHomePhabricator

[LVI] Extend select handling to catch min/max/clamp idioms

Authored by reames on Feb 11 2016, 6:38 PM.



Most of this is fairly straight forward. I was a bit surprised to find we didn't have smin/umin on ConstantRange. Careful review of that would be appreciated.

Note that I'm only handling two constant ranges at this point. It would be reasonable to consider treating overdefined as a full range if the instruction is typed as an integer, but that should be a separate change.

Diff Detail


Event Timeline

reames updated this revision to Diff 47762.Feb 11 2016, 6:38 PM
reames retitled this revision from to [LVI] Extend select handling to catch min/max/clamp idioms.
reames updated this object.
reames added reviewers: hfinkel, sanjoy, nlewycky.
reames added a subscriber: llvm-commits.
ab added a subscriber: ab.Feb 12 2016, 10:46 AM
ab added inline comments.
718 ↗(On Diff #47762)

smax -> smin? Same below

reames added inline comments.Feb 12 2016, 11:18 AM
718 ↗(On Diff #47762)

Yep, stale comment. Will fix, thanks.

sanjoy requested changes to this revision.Feb 25 2016, 10:58 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
972 ↗(On Diff #47762)

Nit: should be AddConstants.

979 ↗(On Diff #47762)

This would be easier to follow if you summarize the transform you're doing / pattern you're matching, like

select (A == CIBase) _ (A + CIAdded)

982 ↗(On Diff #47762)

How can you infer this without looking at the SI->getTrueValue()? Can't the select be select (A == CIBase) (CIBase + CIAdded) (A + CIAdded)? Or is there a check somewhere that I missed?

717 ↗(On Diff #47762)

These should get tests in unittests/IR/ConstantRange.cpp. I'd say split these out, add tests and land them -- the smin and umin implementations lgtm in their current form (with the edit @ab suggested).

This revision now requires changes to proceed.Feb 25 2016, 10:58 PM
reames updated this revision to Diff 49242.Feb 26 2016, 2:28 PM
reames edited edge metadata.

update patches, rebased after separating and landing constant range parts per Sanjoy's comment.

972 ↗(On Diff #47762)

Per style guide:
Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

While a lambda is technically a variable, it's really just a locally scoped function in this usage.

979 ↗(On Diff #47762)

See updated patch. Is that better?

982 ↗(On Diff #47762)

If A != CIBase, then the RHS can't contribute CIBase + CIAdded. The LHS can, but we're only constraining the input ranges here.

sanjoy accepted this revision.Feb 26 2016, 2:41 PM
sanjoy edited edge metadata.


922 ↗(On Diff #49242)

Is it possible for LHS to be the false value and RHS to be the true value?

981 ↗(On Diff #49242)

Makes sense.

This revision is now accepted and ready to land.Feb 26 2016, 2:41 PM
reames added inline comments.Feb 26 2016, 2:56 PM
922 ↗(On Diff #49242)

This is defensive programming in case matchSelectPattern get's smarter.

This revision was automatically updated to reflect the committed changes.