This is an archive of the discontinued LLVM Phabricator instance.

[CVP] Simplify SRem when constantrange abs(lhs) < abs(rhs)
ClosedPublic

Authored by StephenFan on Dec 20 2022, 8:07 AM.

Details

Summary

For srem x, y, if abs(constant range of x) less than abs(constant
range of y), we can simplify it as:
srem x, y => x if y is guaranteed to be positive.
'srem x, y => -x' if y is guaranteed to be negative.

Diff Detail

Event Timeline

StephenFan created this revision.Dec 20 2022, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 8:07 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
StephenFan requested review of this revision.Dec 20 2022, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 8:07 AM
nikic requested changes to this revision.Dec 20 2022, 9:20 AM

Actually, this shows that this is not correct as implemented: You should always simplify to LHS, independent of RHS sign. srem result has the same sign as LHS, not RHS.

This revision now requires changes to proceed.Dec 20 2022, 9:20 AM

Actually, this shows that this is not correct as implemented: You should always simplify to LHS, independent of RHS sign. srem result has the same sign as LHS, not RHS.

Looks like the srem.ll is missing such test, too.

Only simplify LHS.

nikic accepted this revision.Dec 21 2022, 2:13 AM

LGTM

llvm/test/Transforms/CorrelatedValuePropagation/srem.ll
539–540

nit: I think it would be cleaner to return i32 %rem from these tests, rather than using in an icmp that doesn't really seem relevant.

This revision is now accepted and ready to land.Dec 21 2022, 2:13 AM
This revision was landed with ongoing or failed builds.Jan 3 2023, 6:53 AM
This revision was automatically updated to reflect the committed changes.