[PATCH] D68360: PR41162 Implement LKK remainder and divisibility algorithms

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3916-3917
 
+  if (DAG.isKnownNeverZero(N1) && !TLI.isIntDivCheap(VT, Attr)) {
+    if (isConstantOrConstantVector(N1)) {
+      // check if there is a div to combine with rem.
----------------
```
if (!TLI.isIntDivCheap(VT, Attr) && isConstantOrConstantVector(N1) && DAG.isKnownNeverZero(N1)) {
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3920-3922
+      SDNode *DivNode =
+          DAG.getNodeIfExists(DivOpcode, N->getVTList(), {N0, N1});
+      if (!DivNode) {
----------------
Not everything has div-rem operation, this check should only be done if said op exists.
That being said, i'm not sure this check should be here, `DAGCombiner::visitSDIV()` already
has `// sdiv, srem -> sdivrem` fold.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3966
+/// Computation" (LKK)
+SDValue DAGCombiner::foldUREM(SDNode *node) {
+  SDLoc DL(node);
----------------
This isn't folding, it's lowering.
This should be in `TargetLowering.{h,cpp}`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3975
+  } else {
+    FVT = EVT::getIntegerVT(*DAG.getContext(), VT.getScalarSizeInBits() * 2);
+  }
----------------
So no 64-bit remainders on basically everything?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3990-3991
+  // If MUL is unavailable, we cannot proceed in any case.
+  if (!TLI.isOperationLegalOrCustomOrPromote(ISD::MUL, FVT) &&
+      TLI.isOperationExpand(ISD::MUL, FVT))
+    return SDValue();
----------------
I think you want just `isOperationLegalOrCustom()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68360/new/

https://reviews.llvm.org/D68360