[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:5091-5102
+  if (NeedRotLHS && NeedRotRHS)
+    return nullptr; // Not part of a rotate
+
+  // If one side doesn't have a shift, see if we can extract it
+  if (NeedRotRHS) {
+    if (!(RHSShift = extractShiftFromMulOrUDiv(LHSShift, RHS, RHSMask, DL)))
+      return nullptr;
----------------
lebedev.ri wrote:
> Aren't we loosing by bailing out here? (And no tests show it?!)
> Maybe you meant:
> ```
> // If only one side has a shift, see if we can extract it
> if (!(NeedRotLHS ^ NeedRotRHS)) {
>   if (NeedRotRHS) {
>     if (SDValue Shift = extractShiftFromMulOrUDiv(LHSShift, RHS, RHSMask, DL))
>       RHSShift = Shift;
>   }
>   if (NeedRotLHS) {
>     if (SDValue Shift = extractShiftFromMulOrUDiv(RHSShift, LHS, LHSMask, DL))
>       LHSShift = Shift;
>   }
> }
> ```
> Please don't blindly follow that snippet, i don't know what the other code does,
> so your change may be correct as-is.
I'll add some extra comments here and update in a bit, but I don't think we're losing out on anything.  
The idea here is that if MatchRotateHalf matches one half of the OR as a shl/srl but not the other, we try to extract the needed shift from the unmatched side to complete the rotate.  The case where neither side matched (ie NeedRotLHS && NeedRotRHS) is handled above, so at this point either no side is missing and neither of these if cases will fire, or only one side is missing, in which case we try to extract the shift from the missing side (using the shift that did match from the opposite side to compute the shift amount and opcode needed, hence we pass LHSShift when trying to extract from the RHS and vice versa).  If extraction fails at that point then we definitely can't complete the rotate and bail.

For instance if NeedRotRHS==true and LHSShift is (srl v 10) and bitwidth(v)==32, then extractShiftFromMulOrDiv will compute that we need to extract (shl v 22) from RHS based on the shift amount and opcode in LHSShift.


Repository:
  rL LLVM

https://reviews.llvm.org/D47681