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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some more nits.
Overall, this //seems// ok, but i don't feel confident reviewing this as a whole :/



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4901
+  else
+    return SDValue();
+
----------------
lebedev.ri wrote:
> RKSimon wrote:
> > Worth doing this earlier?
> +1 this should be done right `ExtractFrom` is modified.
Not done.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4895-4897
+  if (!OppShiftCst || !OppShiftCst->getAPIntValue() || 
+      !OppLHSCst || !OppLHSCst->getAPIntValue() || 
+      !ExtractFromCst || !ExtractFromCst->getAPIntValue())
----------------
lebedev.ri wrote:
> What does `getAPIntValue()` check? We don't want shifts/mul/div by `0`?
I didn't receive a reply here


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4964
   // See if this is some rotate idiom.
-  if (SDNode *Rot = MatchRotate(N0, N1, SDLoc(N)))
+  if (SDNode *Rot = MatchRotate(N0, N1, SDLoc(N))) {
     return SDValue(Rot, 0);
----------------
Nit: unneeded `{}`


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5037-5039
+  if (OppShiftLHS.getOpcode() != ExtractFrom.getOpcode() ||
+      ShiftedVT != ExtractFrom.getValueType())
+    return SDValue();
----------------
Can't you check `OppShiftLHS.getOperand(0) != ExtractFrom.getOperand(0)` here?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5080
+    return SDValue();
+  // Normalize the bitwdith of the two mul/udiv/shift constant operands
+  APInt ExtractFromAmt = ExtractFromCst->getAPIntValue();
----------------
bitwidth


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5292-5294
+  // If neither side matched a rotate half, bail
+  if (NeedRotLHS && NeedRotRHS)
+    return nullptr;
----------------
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.


================
Comment at: test/CodeGen/X86/rotate-extract.ll:83
+}
+
+; Result would undershift
----------------
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?


https://reviews.llvm.org/D47681