[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:706-707
+  EVT VT = N->getValueType(0);
+  if (VT != MVT::i32 && VT != MVT::i64)
+    return SDValue();
+
----------------
Can you add a comment *here in the code* as to why this restriction exists?



================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:712
+      return false;
+    SDNode *ShiftVal = Shift.getOperand(1).getNode();
+    if (isa<ConstantSDNode>(ShiftVal) &&
----------------
I'm not sure why you need `.getNode()`?
Elsewhere `isa<ConstantSDNode>(SDValue)` works fine.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:713-716
+    if (isa<ConstantSDNode>(ShiftVal) &&
+        (cast<ConstantSDNode>(ShiftVal)->getZExtValue() <= 3))
+      return true;
+    return false;
----------------
Can you add a comment before the `if` as to why the limit is `3`?


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:720-721
+  SDLoc DL(N);
+  unsigned Opcode = N->getOpcode();
+  if (Opcode == ISD::SUB) {
+    SDValue N0 = N->getOperand(0);
----------------
1. This is the only use of `Opcode` variable. Inline it into the usage?
2. Why not early return? It took me a moment to understand that we indeed don't have an `else if` for this `if`.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:774
+    // addressing mode based LEA selection.
+    if (OptForSize) {
+      SDValue NewN = pruneForLEAExtraction(N);
----------------
Hm, why do we want to only do this in `-Os`/`-Oz`?


Repository:
  rL LLVM

https://reviews.llvm.org/D49966