Page MenuHomePhabricator

[SCEV] Simplify umin/max of zext and sext of the same value
ClosedPublic

Authored by reames on Tue, Oct 15, 2:20 PM.

Details

Summary

This is a common idiom which arises after induction variables are widened, and we have two or more exit conditions.

Reviewers, my commented reasoning is sound right? This seems pretty basic, and nothing I can find in LLVM handles this pattern. Am I missing something?

Diff Detail

Event Timeline

reames created this revision.Tue, Oct 15, 2:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 15, 2:20 PM

As the property you're ultimately using here is that zext x u<= sext x, would it make more sense to include that as part of isKnownViaNonRecursiveReasoning()? A corollary is that sext x s<= zext x.

Alive: https://rise4fun.com/Alive/sllu

A bit surprised that this pattern is not picked up by instcombine: https://godbolt.org/z/hP4wyd

reames updated this revision to Diff 225449.Thu, Oct 17, 9:33 AM

Incorporate review comment.

Could you please also include test coverage for the smin/smax case? Otherwise this looks good.

lib/Analysis/ScalarEvolution.cpp
10344 ↗(On Diff #225449)

The implementation of IsKnownPredicateViaMinOrMax() canonicalizes ICMP_SGE/ICMP_UGE by swapping (and isKnownPredicateViaNoOverflow() does as well), so I would suggest including it here as well.

It doesn't matter for the min/max case as both orders are tried there, but generally this code doesn't seem to assume a particular predicate canonicalization.

reames updated this revision to Diff 225545.Thu, Oct 17, 4:35 PM

Address review comments.

p.s. Thank you for really great suggestions here. The new version of the code is much better than the original.

nikic accepted this revision.Fri, Oct 18, 10:57 AM

LGTM

This revision is now accepted and ready to land.Fri, Oct 18, 10:57 AM
This revision was automatically updated to reflect the committed changes.