[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 marked 15 inline comments as done.
sameconrad added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4901
+  else
+    return SDValue();
+
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > RKSimon wrote:
> > > Worth doing this earlier?
> > +1 this should be done right `ExtractFrom` is modified.
> Not done.
I moved the opcode check to the beginning of the function, I believe that was the original request here? The review comments get misaligned after updating the code


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4842-4843
+///   (or (shrl (shrl v c1) c3) (shl (shrl v c1) c2))
+static SDValue extractShift(SelectionDAG &DAG, SDValue OppShift,
+                            SDValue ExtractFrom, SDValue &Mask,
+                            const SDLoc &DL) {
----------------
lebedev.ri wrote:
> You want to add a comment that it will be called for both the LHS and RHS,
> so it does not have to worry about looking at the other side here.
This is mentioned in the comments I added in MatchRotate


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4895-4897
+  if (!OppShiftCst || !OppShiftCst->getAPIntValue() || 
+      !OppLHSCst || !OppLHSCst->getAPIntValue() || 
+      !ExtractFromCst || !ExtractFromCst->getAPIntValue())
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > What does `getAPIntValue()` check? We don't want shifts/mul/div by `0`?
> I didn't receive a reply here
Yes, just checks that is not 0.  A rotate by zero would just be a noop, so it didn't seem to make sense to try and extract anything in that case.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5037-5039
+  if (OppShiftLHS.getOpcode() != ExtractFrom.getOpcode() ||
+      ShiftedVT != ExtractFrom.getValueType())
+    return SDValue();
----------------
lebedev.ri wrote:
> Can't you check `OppShiftLHS.getOperand(0) != ExtractFrom.getOperand(0)` here?
I moved the opcode check to right after stripConstantMask like you mentioned in your other comment, so this operand(0) check comes right after that now.  I think we should verify the ExtractFrom opcode is a shift/mul/udiv before checking operands, otherwise we don't know what the operand count of the instruction is.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5292-5294
+  // If neither side matched a rotate half, bail
+  if (NeedRotLHS && NeedRotRHS)
+    return nullptr;
----------------
lebedev.ri wrote:
> Do you need those extra variables to to track this?
> Maybe you can `s/NeedRotLHS/!LHSShift/`, and drop the variables?
> Otherwise i fear later on they may start meaning different things.
Thanks for pointing that out.  Those bools were completely unneeded.


================
Comment at: test/CodeGen/X86/rotate-extract.ll:83
+}
+
+; Result would undershift
----------------
efriedma wrote:
> lebedev.ri wrote:
> > These are all `rol`'s, and in aarch64 they are all `ror`'s, even though the tests appear identical, interesting.
> > Any way to produce both `ror` and `rol` instructions for both arches?
> AArch64 doesn't have a rol instruction.
The ISD selection in MatchRotate just looks like HasROTL ? ISD::ROTL : ISD::ROTR, so it looks will just always choose a rotl if available and fall back to a rotr like it does on AArch64.  Since X86 has rotl it will always select that, I don't see any way to vary it within the same ISA.


https://reviews.llvm.org/D47681