[PATCH] D50222: [CodeGen] [SelectionDAG] More efficient code for X % C == 0 (UREM case)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

This looks about right as far as i can tell.
But would be best if others could review, too.



================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3727
+                                        const SDLoc &DL) const {
+  // fold (seteq/ne (urem N, D), 0) -> (setule/gte (rotr (mul N, P), K), Q)
+  // - D must be constant with D = D0 * 2^K where D0 is odd
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > ```
> > > assert((Cond == ISD::SETEQ || Cond == ISD::SETNE) &&
> > >        "Only applicable for [in]equality comparisons.");
> > > ```
> > ```
> > // fold (seteq/ne (urem N, D), 0) -> (setule/ugt (rotr (mul N, P), K), Q)
> > 
> > ```
> > (it would be great to use more descriptive names to differentiate between `C`onstants and not constants, too)
> > (it would be great to use more descriptive names to differentiate between Constants and not constants, too)
> 
> But thinking about it a bit more, probably don't rename the variables. Consistency [with the orig doc] may be best.
The comment change wasn't applied, i'm not aware of `gte` predicate, but i do know `[u/s]ge`.
Also, reading the actual code, `ugt` was meant it seems.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3772
+  // where W is the width of the common type of N and D
+  SelectionDAG &DAG = DCI.DAG;
+  EVT REMVT = REMNode->getValueType(0);
----------------
You can move this closer to where it's actually used.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3789
+
+  APInt D = Divisor->getAPIntValue();
+
----------------
You don't actually modify `D` as far as i can tell?
I think it should be `const APInt &D`


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3799
+  APInt D0 = D.lshr(K);
+
+  // P = inv(D0, 2^W)
----------------
Ok, thank you for the explanation regarding `D0 != 1`.
I think here now we can just add an assert, to document it.
```
APInt D0 = D.lshr(K);

assert(!D0.isOneValue() && "The fold is invalid for D0 of 1. Unreachable since we leave powers-of-two to be improved elsewhere.");
```


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3814
+  SDValue Op1 = DAG.getNode(ISD::MUL, DL, REMVT, REMNode->getOperand(0), PVal);
+  // Will change in the signed case
+  SDValue Op2 = QVal;
----------------
Drop the comment about signed cases?
I strongly believe that should be done in a new function.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3824
+    // UREM: (rotr (mul N, P), K)
+    // SREM: (rotr (add (mul N, P), Q'), K)
+    Op1 = DAG.getNode(ISD::ROTR, DL, REMVT, Op1, ShAmt, Flags);
----------------
Drop the comment about signed cases?
I strongly believe that should be done in a new function.


Repository:
  rL LLVM

https://reviews.llvm.org/D50222