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

I can add some nits, but i'm not confident reviewing this as a whole..



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


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4863-4876
+  if (OppShift.getOpcode() == ISD::SRL) {
+    // If op0 is mul or shl and the opposing is an shrl, we can extract a shl
+    IsMulOrDiv = ExtractFrom.getOpcode() == ISD::MUL;
+    if (IsMulOrDiv || ExtractFrom.getOpcode() == ISD::SHL)
+      Opcode = ISD::SHL;
+  } else if (OppShift.getOpcode() == ISD::SHL) {
+    // If op0 is a srl or udiv and the opposing is a shl, we can extract a srl
----------------
This does basically the same (but opposite) thing twice.
I would personally try to do something like
```
auto parse = [&IsMulOrDiv, &Opcode, OppShift, ExtractFrom](unsigned Op0, unsigned Op1, unsigned Op2) -> bool {
  if (OppShift.getOpcode() != Op0)
    return false;
  IsMulOrDiv = ExtractFrom.getOpcode() == Op1;
  if (IsMulOrDiv || ExtractFrom.getOpcode() == Op2)
    Opcode = Op2;
  return true;
};
if(!(parse(ISD::SRL, ISD::MUL,  ISD::SHL) ||
     parse(ISD::SHL, ISD::UDIV, ISD::SRL)) ||
   Opcode == ISD::DELETED_NODE)
  return SDValue();
```
Even if it's the same LOC, it significantly reduced duplication,
allowing to focus on the actual details..


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


https://reviews.llvm.org/D47681