[PATCH] D63963: [Codegen][SelectionDAG] X u% C == 0 fold: non-splat vector improvements

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1588
+  /// Alternatively, replace all the values for which the Predicate does match
+  /// with AlternativeReplacement if it was specified.
+  void turnVectorIntoSplatVector(MutableArrayRef<SDValue> Values,
----------------
This comment doesn't really grok well, if I've understood correctly it should be something like:

"If Values contains values that either match the predicate and the non-matching values are the same 'splat' values, then replace all of them with the 'splat' value."

But I'm not sure that's much better.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:4058
 
-  SDValue prepareUREMEqFold(EVT VT, SDValue N0, SDValue N1, ISD::CondCode Cond,
-                            DAGCombinerInfo &DCI, const SDLoc &DL,
+  SDValue prepareUREMEqFold(EVT VT, SDValue N0, SDValue CompTargetNode,
+                            ISD::CondCode Cond, DAGCombinerInfo &DCI,
----------------
N0 -> REMNode?


================
Comment at: include/llvm/CodeGen/TargetLowering.h:4064
+                          ISD::CondCode Cond, DAGCombinerInfo &DCI,
+                          const SDLoc &DL) const;
 };
----------------
These look like a separate NFC change?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4472
+  if (SDValue Folded = prepareUREMEqFold(VT, REMNode, CompNodeTargetNode, Cond,
+                                         DCI, DL, Built)) {
     for (SDNode *N : Built)
----------------
Separate NFC change?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4485
+                                  DAGCombinerInfo &DCI, const SDLoc &DL,
                                   SmallVectorImpl<SDNode *> &Created) const {
   // fold (seteq/ne (urem N, D), 0) -> (setule/ugt (rotr (mul N, P), K), Q)
----------------
Separate NFC change?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4496
 
-  SDValue PVal = DAG.getConstant(P, DL, REMVT);
-  SDValue QVal = DAG.getConstant(Q, DL, REMVT);
-  // (mul N, P)
-  SDValue Op1 = DAG.getNode(ISD::MUL, DL, REMVT, REMNode->getOperand(0), PVal);
-  Created.push_back(Op1.getNode());
+  EVT VT = REMNode->getValueType(0);
+  EVT SVT = VT.getScalarType();
----------------
REMNode.getValueType() ?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4594
+                                    DAG.getConstant(0, DL, ShSVT));
+    }
+
----------------
Is this code really necessary? Can't you just use SetCC/Select like we do in the BuildDIV methods? SimplifyDemandedBits/SimplifyDemandedVectorElts are more likely to deal with it then.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63963