[PATCH] D47681: [DAGCombiner] Bug 31275- Extract a shift from a constant mul or udiv if a rotate can be formed

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

The tests probably need committing with trunk's current codegen, so this patch then shows the diff.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4822
   if (Op.getOpcode() == ISD::AND) {
     if (DAG.isConstantIntBuildVectorOrConstantInt(Op.getOperand(1))) {
       Mask = Op.getOperand(1);
----------------
You can merge these if statements:
```
if (Op.getOpcode() == ISD::AND &&
    DAG.isConstantIntBuildVectorOrConstantInt(Op.getOperand(1))) {
```


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4831
+/// Match "(X shl/srl V1) & V2" where V2 may not be present.
+bool DAGCombiner::MatchRotateHalf(SDValue Op, SDValue &Shift, SDValue &Mask) {
+  Op = stripConstantMask(DAG, Op, Mask);
----------------
Does this need to be a DAGCombiner method any more? Just reduce it to a static.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4842
 
+SDValue DAGCombiner::extractShiftFromMulOrUDiv(SDValue OpposingShift,
+                                               SDValue MulOrDiv, SDValue &Mask,
----------------
Again, what do you gain from this being a DAGCombiner method?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4850
+
+  MulOrDiv = stripConstantMask(DAG, MulOrDiv, Mask);
+  SDValue ShiftLHS = OpposingShift.getOperand(0);
----------------
Do we have any tests that use the Mask?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4878
+  if (OppShiftAmt == 0 | ShiftLHSMulOrDivAmount == 0 || MulOrDivAmount == 0)
+    return SDValue();
+
----------------
This is more convoluted than it needs to be - the GetConstant isn't really helping and comparing against APInt() is a bad idea 

Would it be better to just do (not sure about this):
```
// Attempt to find non-zero constants for opposing shift and the mul/div +shift.
ConstantSDNode *OppShiftCst = isConstOrConstSplat(OpposingShift.getOperand(1));
ConstantSDNode *ShiftCst = isConstOrConstSplat(ShiftLHS.getOperand(1));
ConstantSDNode *MulOrDivCst = isConstOrConstSplat(MulOrDiv.getOperand(1));
if (!OppShiftCst || !ShiftCst !! MulOrDivCst ||
    !OppShiftCst->getAPIntValue() || !ShiftCst->getAPIntValue() || !MulOrDivCst->getAPIntValue())
  return SDValue();
```

Also, we should bail if the OppShiftCst is out of range.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4885
+  const APInt NewShiftAmt = VTWidth - OppShiftAmt;
+  const APInt ShiftExtractDiv = APInt(VTWidth, 1) << NewShiftAmt;
+  APInt NewMulOrDivAmount;
----------------
Is this better?
```
APInt ShiftExtractDiv = APInt::getOneBitSet(VTWidth, NewShiftAmt;.getZextValue());
```


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4901
+  else
+    return SDValue();
+
----------------
Worth doing this earlier?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5116
+      return nullptr;
+  }
+
----------------
This could be reduced to:

```
  if (NeedRotRHS)
    RHSShift = extractShiftFromMulOrUDiv(LHSShift, RHS, RHSMask, DL);
  if (NeedRotLHS)
    LHSShift = extractShiftFromMulOrUDiv(RHSShift, LHS, LHSMask, DL);
  if (!RHSShift || !LHSShift)
    return nullptr;
```


https://reviews.llvm.org/D47681