[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 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;
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5096-5100
+    if (!(RHSShift = extractShiftFromMulOrUDiv(LHSShift, RHS, RHSMask, DL)))
+      return nullptr;
+  }
+  if (NeedRotLHS) {
+    if (!(LHSShift = extractShiftFromMulOrUDiv(RHSShift, LHS, LHSMask, DL)))
----------------
Also, it is intentional that you extract from LHS and store it into a variable named RHS and vice-versa?
If yes, add a comment please.


================
Comment at: test/CodeGen/X86/rotate-extract.ll:1
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s
+
----------------
You might want to use `llvm/utils/update_llc_test_checks.py`


Repository:
  rL LLVM

https://reviews.llvm.org/D47681