[ScheduleDAG] Fix unfolding of SUnits to already existent nodes.

[niravd]

Summary

If unfolding an SUnit results in both load or the operation using it which
already exist in the DAG, abort the unfold if they are already scheduled.
If not, make sure we don't add duplicate dependencies.

This fixes PR37916.

[niravd]

Ping

[niravd]

Ping

[efriedma]

LGTM with one nit

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

1013 ↗

(On Diff #153150)

Please add a comment here noting that we specifically end up in this situation when isNewLoad == false.

llvm/trunk/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

SUnit *ScheduleDAGRRList::TryUnfoldSU(SUnit *SU) {
   } else {
     LoadSU = CreateNewSUnit(LoadNode);
     LoadNode->setNodeId(LoadSU->NodeNum);
 
     InitNumRegDefsLeft(LoadSU);
     computeLatency(LoadSU);
   }
 
-  LLVM_DEBUG(dbgs() << "Unfolding SU #" << SU->NodeNum << "\n");
-
-  // Now that we are committed to unfolding replace DAG Uses.
-  for (unsigned i = 0; i != NumVals; ++i)
-    DAG->ReplaceAllUsesOfValueWith(SDValue(SU->getNode(), i), SDValue(N, i));
-  DAG->ReplaceAllUsesOfValueWith(SDValue(SU->getNode(), OldNumVals - 1),
-                                 SDValue(LoadNode, 1));
-
-  SUnit *NewSU = CreateNewSUnit(N);
-  assert(N->getNodeId() == -1 && "Node already inserted!");
+  bool isNewN = true;
+  SUnit *NewSU;
+  // This can only happen when isNewLoad is false.
+  if (N->getNodeId() != -1) {
+    NewSU = &SUnits[N->getNodeId()];
+    // If NewSU has already been scheduled, we need to clone it, but this
+    // negates the benefit to unfolding so just return SU.
+    if (NewSU->isScheduled)
+      return SU;
+    isNewN = false;
+  } else {
+    NewSU = CreateNewSUnit(N);
     N->setNodeId(NewSU->NodeNum);
 
     const MCInstrDesc &MCID = TII->get(N->getMachineOpcode());
     for (unsigned i = 0; i != MCID.getNumOperands(); ++i) {
       if (MCID.getOperandConstraint(i, MCOI::TIED_TO) != -1) {
         NewSU->isTwoAddress = true;
         break;
       }
     }
     if (MCID.isCommutable())
       NewSU->isCommutable = true;
 
     InitNumRegDefsLeft(NewSU);
     computeLatency(NewSU);
+  }
+
+  LLVM_DEBUG(dbgs() << "Unfolding SU #" << SU->NodeNum << "\n");
+
+  // Now that we are committed to unfolding replace DAG Uses.
+  for (unsigned i = 0; i != NumVals; ++i)
+    DAG->ReplaceAllUsesOfValueWith(SDValue(SU->getNode(), i), SDValue(N, i));
+  DAG->ReplaceAllUsesOfValueWith(SDValue(SU->getNode(), OldNumVals - 1),
+                                 SDValue(LoadNode, 1));
 
   // Record all the edges to and from the old SU, by category.
   SmallVector<SDep, 4> ChainPreds;
   SmallVector<SDep, 4> ChainSuccs;
   SmallVector<SDep, 4> LoadPreds;
   SmallVector<SDep, 4> NodePreds;
   SmallVector<SDep, 4> NodeSuccs;
   for (SDep &Pred : SU->Preds) {
SUnit *ScheduleDAGRRList::TryUnfoldSU(SUnit *SU) {
   // Add a data dependency to reflect that NewSU reads the value defined
   // by LoadSU.
   SDep D(LoadSU, SDep::Data, 0);
   D.setLatency(LoadSU->Latency);
   AddPred(NewSU, D);
 
   if (isNewLoad)
     AvailableQueue->addNode(LoadSU);
+  if (isNewN)
     AvailableQueue->addNode(NewSU);
 
   ++NumUnfolds;
 
   if (NewSU->NumSuccsLeft == 0)
     NewSU->isAvailable = true;
 
   return NewSU;
 }

llvm/trunk/test/CodeGen/X86/pr37916.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=i386-unknown-linux-gnu %s -o - | FileCheck %s
+
+@f = external local_unnamed_addr global i64*, align 4
+@a = external global i64, align 8
+
+define void @fn1() local_unnamed_addr {
+; CHECK-LABEL: fn1:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    .p2align 4, 0x90
+; CHECK-NEXT:  .LBB0_1: # %if.end
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    movl a+4, %eax
+; CHECK-NEXT:    orl a, %eax
+; CHECK-NEXT:    movl $a, f
+; CHECK-NEXT:    je .LBB0_3
+; CHECK-NEXT:  # %bb.2: # %if.end
+; CHECK-NEXT:    # in Loop: Header=BB0_1 Depth=1
+; CHECK-NEXT:    jne .LBB0_1
+; CHECK-NEXT:  .LBB0_3: # %cond.false
+entry:
+  br label %if.end
+
+if.end:                                           ; preds = %cond.end, %entry
+  br label %if.then7
+
+if.then7:                                         ; preds = %if.end
+  store i64* @a, i64** @f, align 4
+  %0 = load i64*, i64** @f, align 4
+  %1 = load i64, i64* %0, align 4
+  %tobool12 = icmp ne i64 %1, 0
+  %2 = load i64, i64* @a, align 8
+  %tobool13 = icmp ne i64 %2, 0
+  %3 = and i1 %tobool12, %tobool13
+  %tobool14.demorgan = and i1 %tobool12, %tobool13
+  br i1 %tobool14.demorgan, label %cond.end, label %cond.false
+
+cond.false:                                       ; preds = %if.then7
+  unreachable
+
+cond.end:                                         ; preds = %if.then7
+  %conv17 = sext i1 %3 to i8
+  br label %if.end
+}