[PATCH] D61047: [X86] Delay creating index register negations during address matching until after we know for sure the match will succeed

[Craig Topper via Phabricator via llvm-commits]

craig.topper created this revision.
craig.topper added reviewers: niravd, RKSimon, spatel.
Herald added subscribers: arphaman, hiraditya.
Herald added a project: LLVM.

If we're trying to match an LEA, its possible the LEA match will be deemed unprofitable. In which case the negation we created in matchAddress would be left dangling in the SelectionDAG. This could artificially increase use counts for other nodes in the DAG. Though I don't have an example of that. But it just seems like bad form to have dangling nodes in isel.


https://reviews.llvm.org/D61047

Files:
  llvm/lib/Target/X86/X86ISelDAGToDAG.cpp


Index: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
===================================================================
--- llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -73,6 +73,7 @@
     int JT;
     unsigned Align;    // CP alignment.
     unsigned char SymbolFlags;  // X86II::MO_*
+    bool NegateIndex = false;
 
     X86ISelAddressMode()
         : BaseType(RegBase), Base_FrameIndex(0), Scale(1), IndexReg(), Disp(0),
@@ -115,6 +116,8 @@
         dbgs() << " Base.FrameIndex " << Base_FrameIndex << '\n';
       dbgs() << " Scale " << Scale << '\n'
              << "IndexReg ";
+      if (NegateIndex)
+        dbgs() << "negate ";
       if (IndexReg.getNode())
         IndexReg.getNode()->dump(DAG);
       else
@@ -261,6 +264,7 @@
                                    SDValue &Base, SDValue &Scale,
                                    SDValue &Index, SDValue &Disp,
                                    SDValue &Segment) {
+      assert(!AM.NegateIndex && "Index should have been negated!");
       Base = (AM.BaseType == X86ISelAddressMode::FrameIndexBase)
                  ? CurDAG->getTargetFrameIndex(
                        AM.Base_FrameIndex,
@@ -1837,14 +1841,11 @@
     }
 
     // Ok, the transformation is legal and appears profitable. Go for it.
-    SDValue Zero = CurDAG->getConstant(0, dl, N.getValueType());
-    SDValue Neg = CurDAG->getNode(ISD::SUB, dl, N.getValueType(), Zero, RHS);
-    AM.IndexReg = Neg;
+    // Negation will be emitted later to avoid creating dangling nodes if this
+    // was an unprofitable LEA.
+    AM.IndexReg = RHS;
+    AM.NegateIndex = true;
     AM.Scale = 1;
-
-    // Insert the new nodes into the topological ordering.
-    insertDAGNode(*CurDAG, N, Zero);
-    insertDAGNode(*CurDAG, N, Neg);
     return false;
   }
 
@@ -2064,6 +2065,18 @@
   if (!AM.IndexReg.getNode())
     AM.IndexReg = CurDAG->getRegister(0, VT);
 
+  // Negate the index if needed.
+  if (AM.NegateIndex) {
+    SDLoc dl(N);
+    SDValue Zero = CurDAG->getConstant(0, dl, N.getValueType());
+    SDValue Neg = CurDAG->getNode(ISD::SUB, dl, N.getValueType(), Zero,
+                                  AM.IndexReg);
+    AM.IndexReg = Neg;
+    AM.NegateIndex = false;
+    insertDAGNode(*CurDAG, N, Zero);
+    insertDAGNode(*CurDAG, N, Neg);
+  }
+
   getAddressOperands(AM, SDLoc(N), Base, Scale, Index, Disp, Segment);
   return true;
 }
@@ -2283,6 +2296,18 @@
   if (Complexity <= 2)
     return false;
 
+  // Negate the index if needed.
+  if (AM.NegateIndex) {
+    SDLoc dl(N);
+    SDValue Zero = CurDAG->getConstant(0, dl, N.getValueType());
+    SDValue Neg = CurDAG->getNode(ISD::SUB, dl, N.getValueType(), Zero,
+                                  AM.IndexReg);
+    AM.IndexReg = Neg;
+    AM.NegateIndex = false;
+    insertDAGNode(*CurDAG, N, Zero);
+    insertDAGNode(*CurDAG, N, Neg);
+  }
+
   getAddressOperands(AM, DL, Base, Scale, Index, Disp, Segment);
   return true;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61047.196357.patch
Type: text/x-patch
Size: 2970 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190424/21751ca0/attachment.bin>