[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.

Starting to look much better. Some more nits.



================
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
----------------
```
assert((Cond == ISD::SETEQ || Cond == ISD::SETNE) &&
       "Only applicable for [in]equality comparisons.");
```


================
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:
> ```
> 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)


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3749
+
+  // Rewrite D = D0 * 2^K
+  unsigned K = D.countTrailingZeros();
----------------
```
// Decompose D into D0 * 2^K
```


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3750
+  // Rewrite D = D0 * 2^K
+  unsigned K = D.countTrailingZeros();
+  APInt D0 = D.lshr(K);
----------------
```
bool DivisorIsEven = K != 0;
```


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3753-3755
+  // If D0 == 1, we cannot build this fold. Otherwise, D0 > 1, as needed.
+  if (D0.isOneValue())
+    return SDValue();
----------------
So what we are really checking here, is that `D` is not power of two.
How about simplifying this into:
```
APInt D = Divisor->getAPIntValue();
unsigned W = D.getBitWidth();

// If D is power of two, D0 would be 1, and we cannot build this fold.
if(D.isPowerOf2())
  return SDValue();

// Decompose D into D0 * 2^K
...
```

BUT. I do not understand from where does this requirement comes from?
Can you quote specific part of [[ https://doc.lagout.org/security/Hackers%20Delight.pdf | `10–17 Test for Zero Remainder after Division by a Constant` ]] that warrants this?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3757-3761
+  // Calculate the multiplicative inverse P of D0 using Newton's method.
+  APInt tmp;
+  APInt P = D0;
+  while ((tmp = D0 * P) != 1)
+    P *= APInt(D0.getBitWidth(), 2) - tmp;
----------------
Would [[ http://llvm.org/doxygen/classllvm_1_1APInt.html#a28ee15b0286415cce5599d4fb9f9ce02 | `APInt::multiplicativeInverse()` ]] work here?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3764-3765
+  // Q = floor((2^W - 1) / D0)
+  APInt Q = APInt::getAllOnesValue(W);
+  Q = Q.udiv(D0);
+
----------------
I would think just writing this as one line would be as clean
```
APInt Q = APInt::getAllOnesValue(W).udiv(D0);
```


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3771
+  SDValue Op1 = DAG.getNode(ISD::MUL, DL, REMVT, REMNode->getOperand(0), PVal);
+  DCI.AddToWorklist(Op1.getNode());
+  // Will change in the signed case
----------------
I do not know if this is needed?
At least rL338079 removed such a call.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3776
+  // Rotate right only if D was even and thus shifted (K > 0)
+  if (K) {
+    SDValue ShAmt =
----------------
```
if(DivisorIsEven) {
```


Repository:
  rL LLVM

https://reviews.llvm.org/D50222