[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
llvm-commits at lists.llvm.org
Fri Jul 6 18:10:53 PDT 2018
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
More information about the llvm-commits
mailing list