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

[Sam Conrad via Phabricator via llvm-commits]

sameconrad added inline comments.


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


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4878
+  if (OppShiftAmt == 0 | ShiftLHSMulOrDivAmount == 0 || MulOrDivAmount == 0)
+    return SDValue();
+
----------------
RKSimon wrote:
> 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.
Fixed up most of this but wasn't sure what you meant by out of range, do you mean if the existing shift amount is greater than the bitwidth?


https://reviews.llvm.org/D47681