This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Introduce bulk umin creation utilities
ClosedPublic

Authored by skatkov on Apr 25 2018, 2:04 AM.

Details

Summary

Add new umin creation method which accept a list of operands.

SCEV does not represents umin which is required in getExact, so
it transforms umin to umax with not. As a result the transformation of
tree of max to max with several operands does not work.
We just use the new introduced method for creation umin from several operands.

Diff Detail

Repository
rL LLVM

Event Timeline

skatkov created this revision.Apr 25 2018, 2:04 AM
javed.absar added inline comments.Apr 25 2018, 2:16 AM
lib/Analysis/ScalarEvolution.cpp
3566 ↗(On Diff #143873)

There is only x,y here (LHS,RHS), not z as in the comment. Am i missing something here?

My bad incorrect place to update comment. Will update.
Thanks for noticing.

sanjoy accepted this revision.Apr 25 2018, 9:54 AM
sanjoy added inline comments.
lib/Analysis/ScalarEvolution.cpp
3552 ↗(On Diff #143873)

Please also change these binary functions to call into the general n-ary getSMinExpr etc (like we do for e.g. getAddExpr).

6715 ↗(On Diff #143873)

Probably better to also add an n-ary version of getUMinFromMismatchedTypes instead of repeating the logic here?

This revision is now accepted and ready to land.Apr 25 2018, 9:54 AM
skatkov updated this revision to Diff 144050.Apr 25 2018, 6:52 PM

Handled comments.

mkazantsev accepted this revision.Apr 26 2018, 6:21 PM

LGTM too. The same should be done for max.

lib/Analysis/ScalarEvolution.cpp
3967 ↗(On Diff #144050)

Maybe add a fast path for Ops.size() == 1? I'm OK if it goes as a separate NFC.

mkazantsev added inline comments.Apr 26 2018, 6:23 PM
lib/Analysis/ScalarEvolution.cpp
3967 ↗(On Diff #144050)

Also please assert on Ops.empty()

This revision was automatically updated to reflect the committed changes.