[PATCH] D49966: [X86] Performing DAG pruning before selection of LEA instructions.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:705-707
+  SDLoc DL(N);
+  EVT VT = N->getValueType(0);
+  unsigned Opcode = N->getOpcode();
----------------
These can be moved to after the lambda.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:708
+  unsigned Opcode = N->getOpcode();
+  auto IsValidConstantShift = [=](SDValue Shift) {
+    SDNode *ShiftVal = Shift.getOperand(1).getNode();
----------------
But this doesn't check that it's *argument* is ValidConstantShift.
It checks that the `getOperand(1).getNode()` of the argument is `ValidConstantShift`.

How about you sink the `Opr0.getOpcode() == ISD::SHL` check into the lambda, and rename the lambda somehow?


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:710-711
+    SDNode *ShiftVal = Shift.getOperand(1).getNode();
+    if (isa<ConstantSDNode>(ShiftVal) &&
+        (cast<ConstantSDNode>(ShiftVal)->getZExtValue() <= 8))
+      return true;
----------------
Is this sufficient to limit this to scalars only?
Also, should this be limited to only some specific `EVT`'s?


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:716
+
+  if (Opcode == ISD::SUB) {
+    SDValue Opr0 = N->getOperand(0);
----------------
Are there other patterns with different root nodes possible? If not i'd early-return.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:717-718
+  if (Opcode == ISD::SUB) {
+    SDValue Opr0 = N->getOperand(0);
+    SDValue Opr1 = N->getOperand(1);
+    if (Opr0.getOpcode() == ISD::SHL && IsValidConstantShift(Opr0)) {
----------------
General naming comment: it seems, these would normally be named `N0` and `N1`, because they are 0'th and 1'th operands of `N`.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:744
+    }
+  }
+  return false;
----------------
Any thoughts on whether the `shl` should be one-use?


Repository:
  rL LLVM

https://reviews.llvm.org/D49966