This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][SelectionDAG] More efficient code for X % C == 0 (SREM case)
ClosedPublic

Authored by lebedev.ri on Jul 26 2019, 7:58 PM.

Details

Summary

This implements an optimization described in Hacker's Delight 10-17:
when C is constant, the result of X % C == 0 can be computed
more cheaply without actually calculating the remainder.
The motivation is discussed here: https://bugs.llvm.org/show_bug.cgi?id=35479.

One huge caveat: this signed case is only valid for positive divisors.

While we can freely negate negative divisors, we can't negate INT_MIN,
so for now if INT_MIN is encountered, we bailout.
As a follow-up, it should be possible to handle that more gracefully
via extra and+setcc+select.

This passes llvm's test-suite, and from cursory(!) cross-examination
the folds (the assembly) match those of GCC, and manual checking via alive
did not reveal any issues (other than the INT_MIN case)

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jul 26 2019, 7:58 PM
RKSimon added inline comments.Jul 27 2019, 7:19 AM
llvm/include/llvm/ADT/APInt.h
1476 ↗(On Diff #212041)

Pull this out and add a unit test? Ideally this could be optimized a lot more.

lebedev.ri marked an inline comment as done.

Pulled APInt change into D65369.

craig.topper added inline comments.Jul 29 2019, 2:58 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
4796 ↗(On Diff #212060)

Would rem %X, -C -> rem %X, C even be valid for urem?

5009 ↗(On Diff #212060)

Is there a trick here or is one of these a typo? The comment says 2*A and the code says 3*A.

lebedev.ri marked an inline comment as done.EditedJul 29 2019, 3:13 PM

It may have been a good idea to post that particular diff after sleeping on it, not in sleep-deprived state :S

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
4796 ↗(On Diff #212060)

Uhm, no, i was going to drop the comment but forgot about it. Thanks.

5009 ↗(On Diff #212060)

This is a bug, i was trying to see if this would trigger any failures in test-suite (i think it didn't),
and forgot to tidy the diff before posting :/

lebedev.ri marked 4 inline comments as done.

Updated:

  • Improved test coverage - negative divisors, INT_MIN divisors
  • Rebased
  • Fix review notes
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5009 ↗(On Diff #212060)

Double-checked, and indeed this does not break test-suite.

X86 changes look good.

xbolva00 accepted this revision.Aug 13 2019, 3:02 AM
xbolva00 added a subscriber: xbolva00.

Looks fine

@RKSimon ?

This revision is now accepted and ready to land.Aug 13 2019, 3:02 AM
lebedev.ri marked an inline comment as done.Aug 13 2019, 3:06 AM
lebedev.ri added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
4964 ↗(On Diff #212297)

This should be SmallVector<SDNode *, 3>, will fix during next update.

RKSimon accepted this revision.Aug 13 2019, 5:54 AM

LGTM with a couple of minor observations

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3761 ↗(On Diff #212297)

Worth doing this as an if/else with the UREM compare?

4816 ↗(On Diff #212297)

separate this off

This revision was automatically updated to reflect the committed changes.