[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
llvm-commits at lists.llvm.org
Mon Jun 11 02:54:21 PDT 2018
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
More information about the llvm-commits
mailing list