[PATCH] D61164: [X86] If PreprocessISelDAG reorders a load before a call, make sure we remove dead nodes from the graph

[Craig Topper via Phabricator via llvm-commits]

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

The reordering can leave at least a dead TokenFactor in the graph. This cause the linearize scheduler to fail with something like the assert seen in PR22614. This is only one of many ways we can break the linearize scheduler today so I can't say for sure that any of the other failures in that bug were caused by this issue.

I took the heavy hammer approach of just running the full RemoveDeadNodes function which will search for dead nodes. We might be able to be more precise and remove exactly the dead TokenFactor or detect that the dead TokenFactor even exists, but I wanted to start with the simplest fix.


https://reviews.llvm.org/D61164

Files:
  llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
  llvm/test/CodeGen/X86/fold-call-3.ll


Index: llvm/test/CodeGen/X86/fold-call-3.ll
===================================================================
--- llvm/test/CodeGen/X86/fold-call-3.ll
+++ llvm/test/CodeGen/X86/fold-call-3.ll
@@ -1,5 +1,7 @@
 ; RUN: llc < %s -mtriple=x86_64-apple-darwin | grep call | grep 560
 ; rdar://6522427
+; This command line used to crash due to dangling nodes left after PreprocessISelDAG
+; RUN: llc < %s -mtriple=x86_64-apple-darwin -pre-RA-sched=linearize
 
 	%"struct.clang::Action" = type { %"struct.clang::ActionBase" }
 	%"struct.clang::ActionBase" = type { i32 (...)** }
Index: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
===================================================================
--- llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -755,6 +755,8 @@
 }
 
 void X86DAGToDAGISel::PreprocessISelDAG() {
+  bool RemoveDeadNodes = false;
+
   for (SelectionDAG::allnodes_iterator I = CurDAG->allnodes_begin(),
        E = CurDAG->allnodes_end(); I != E; ) {
     SDNode *N = &*I++; // Preincrement iterator to avoid invalidation issues.
@@ -829,6 +831,10 @@
         continue;
       moveBelowOrigChain(CurDAG, Load, SDValue(N, 0), Chain);
       ++NumLoadMoved;
+      // This transform can leave dead token factor nodes. Make sure we find
+      // and remove them.
+      // FIXME: Should we be more precise on when to do this?
+      RemoveDeadNodes = true;
       continue;
     }
 
@@ -899,6 +905,9 @@
     ++I;
     CurDAG->DeleteNode(N);
   }
+
+  if (RemoveDeadNodes)
+    CurDAG->RemoveDeadNodes();
 }
 
 // Look for a redundant movzx/movsx that can occur after an 8-bit divrem.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61164.196760.patch
Type: text/x-patch
Size: 1630 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190425/1a031de6/attachment.bin>