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

[craig.topper]

Summary

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.

[niravd]

I worry this is just fix just the dangling nodes as we see them. A brief glance at the AND construction gives us space for this to happen again. I'd much rather we do the removal check unconditionally or leverage something like
DAGUpdateListener to get a more fundamental measure of what nodes may be prunable.

[craig.topper]

By AND construction, I assume you're referring to turning X86ISD::AND into ISD::AND? What's wrong with that code? It's already removing the node it left dead. Or is there a CSE issue?

[niravd]

I was thinking things pending from the folding optimization in getNode, but I missed that it was the target specific one so it should be fine. That said, I still think it's better to do it unconditionally so it'll be pruned by default with the future additio of Listener-based pruning if that proves too expensive.

In D61164#1480551, @craig.topper wrote:

By AND construction, I assume you're referring to turning X86ISD::AND into ISD::AND? What's wrong with that code? It's already removing the node it left dead. Or is there a CSE issue?

[craig.topper]

The AND code is creating target independent ISD::AND from target specific X86ISD::AND so it could be subject to getNode optimization.

Should I make it completely uncondition or make all of the modifications in this loop set the RemoveDeadNodes flag?

[niravd]

I think, make it unconditional.

[craig.topper]

Remove dead nodes unconditionally

[niravd]

LGTM.

llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp

for (SelectionDAG::allnodes_iterator I = CurDAG->allnodes_begin(),
     --I;
     CurDAG->ReplaceAllUsesOfValueWith(SDValue(N, 0), Result);
 
     // Now that we did that, the node is dead.  Increment the iterator to the
     // next node to process, then delete N.
     ++I;
     CurDAG->DeleteNode(N);
   }
+
+  // The load+call transform above can leave some dead nodes in the graph. Make
+  // sure we remove them. Its possible some of the other transforms do to so
+  // just remove dead nodes unconditionally.
+  CurDAG->RemoveDeadNodes();
 }
 
 // Look for a redundant movzx/movsx that can occur after an 8-bit divrem.
 bool X86DAGToDAGISel::tryOptimizeRem8Extend(SDNode *N) {
   unsigned Opc = N->getMachineOpcode();
   if (Opc != X86::MOVZX32rr8 && Opc != X86::MOVSX32rr8 &&
       Opc != X86::MOVSX64rr8)
     return false;

llvm/trunk/test/CodeGen/X86/fold-call-3.ll

 ; 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 (...)** }
   %"struct.clang::ActionBase::ActionResult<0u>" = type { i8*, i8 }
 @NumTrials = internal global i32 10000000   ; <i32*> [#uses=2]
 @llvm.used = appending global [1 x i8*] [ i8* bitcast (void (i8*, %"struct.clang::Action"*)* @_Z25RawPointerPerformanceTestPvRN5clang6ActionE to i8*) ], section "llvm.metadata"    ; <[1 x i8*]*> [#uses=0]
 
 define void @_Z25RawPointerPerformanceTestPvRN5clang6ActionE(i8* %Val, %"struct.clang::Action"* %Actions) nounwind {