ScheduleDAGInstrs: Bug fix for missed memory dependency.

[gberry]

Summary

In buildSchedGraph(), when adding memory dependencies for loads, move
the call to adjustChainDeps() after the call to
addChainDependency(AliasChain) to handle the case where
addChainDependency(AliasChain) ends up not adding a dependency and
instead putting the SU on the RejectMemNodes list. The call to
adjustChainDeps() must be done after the call to addChainDependency() in
order to process the SU added to the RejectMemNodes list to create
memory dependencies for it.

[mcrosier]

@atrick @resistor: Andy/Owen, It would be great if either of you could take a look at this fix! Thanks, Chad.

[atrick]

LGTM with my suggestion for a comment.

lib/CodeGen/ScheduleDAGInstrs.cpp

1140 ↗

(On Diff #44125)

I would be nice to add a short comment explaining why addChainDependency and adjustChainDeps need to be called in this order.

llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp

new_alias_chain:
                              I->second[i], RejectMemNodes);
       }
       for (MapVector<ValueType, std::vector<SUnit *> >::iterator I =
            AliasMemUses.begin(), E = AliasMemUses.end(); I != E; ++I) {
         for (unsigned i = 0, e = I->second.size(); i != e; ++i)
           addChainDependency(AAForDep, MFI, MF.getDataLayout(), SU,
                              I->second[i], RejectMemNodes, TrueMemOrderLatency);
       }
+      // This call must come after calls to addChainDependency() since it
+      // consumes the 'RejectMemNodes' list that addChainDependency() possibly
+      // adds to.
       adjustChainDeps(AA, MFI, MF.getDataLayout(), SU, &ExitSU, RejectMemNodes,
                       TrueMemOrderLatency);
       PendingLoads.clear();
       AliasMemDefs.clear();
       AliasMemUses.clear();
     } else if (MI->mayStore()) {
       // Add dependence on barrier chain, if needed.
       // There is no point to check aliasing on barrier event. Even if
new_alias_chain:
           addChainDependency(AAForDep, MFI, MF.getDataLayout(), SU,
                              PendingLoads[k], RejectMemNodes,
                              TrueMemOrderLatency);
         // Add dependence on alias chain, if needed.
         if (AliasChain)
           addChainDependency(AAForDep, MFI, MF.getDataLayout(), SU, AliasChain,
                              RejectMemNodes);
       }
+      // This call must come after calls to addChainDependency() since it
+      // consumes the 'RejectMemNodes' list that addChainDependency() possibly
+      // adds to.
       adjustChainDeps(AA, MFI, MF.getDataLayout(), SU, &ExitSU, RejectMemNodes,
                       TrueMemOrderLatency);
     } else if (MI->mayLoad()) {
       bool MayAlias = true;
       if (MI->isInvariantLoad(AA)) {
         // Invariant load, no chain dependencies needed!
       } else {
         UnderlyingObjectsVector Objs;
new_alias_chain:
             for (unsigned i = 0, e = I->second.size(); i != e; ++i)
               addChainDependency(AAForDep, MFI, MF.getDataLayout(), SU,
                                  I->second[i], RejectMemNodes, 0, true);
           if (ThisMayAlias)
             AliasMemUses[V].push_back(SU);
           else
             NonAliasMemUses[V].push_back(SU);
         }
-        if (MayAlias)
-          adjustChainDeps(AA, MFI, MF.getDataLayout(), SU, &ExitSU,
-                          RejectMemNodes, /*Latency=*/0);
         // Add dependencies on alias and barrier chains, if needed.
         if (MayAlias && AliasChain)
           addChainDependency(AAForDep, MFI, MF.getDataLayout(), SU, AliasChain,
                              RejectMemNodes);
+        if (MayAlias)
+          // This call must come after calls to addChainDependency() since it
+          // consumes the 'RejectMemNodes' list that addChainDependency()
+          // possibly adds to.
+          adjustChainDeps(AA, MFI, MF.getDataLayout(), SU, &ExitSU,
+                          RejectMemNodes, /*Latency=*/0);
         if (BarrierChain)
           BarrierChain->addPred(SDep(SU, SDep::Barrier));
       }
     }
   }
   if (DbgMI)
     FirstDbgValue = DbgMI;
 

llvm/trunk/test/CodeGen/AArch64/arm64-misched-memdep-bug.ll

+; REQUIRES: asserts
+; RUN: llc < %s -mtriple=arm64-linux-gnu -mcpu=cortex-a57 -enable-misched -verify-misched -debug-only=misched -o - 2>&1 > /dev/null | FileCheck %s
+;
+; Test for bug in misched memory dependency calculation.
+;
+; CHECK: ********** MI Scheduling **********
+; CHECK: misched_bug:BB#0 entry
+; CHECK: SU(2):   %vreg2<def> = LDRWui %vreg0, 1; mem:LD4[%ptr1_plus1] GPR32:%vreg2 GPR64common:%vreg0
+; CHECK:   Successors:
+; CHECK-NEXT:    val SU(5): Latency=4 Reg=%vreg2
+; CHECK-NEXT:    ch  SU(4): Latency=0
+; CHECK: SU(4):   STRWui %WZR, %vreg1, 0; mem:ST4[%ptr2] GPR64common:%vreg1
+; CHECK: SU(5):   %W0<def> = COPY %vreg2; GPR32:%vreg2
+; CHECK: ** ScheduleDAGMI::schedule picking next node
+define i32 @misched_bug(i32* %ptr1, i32* %ptr2) {
+entry:
+  %ptr1_plus1 = getelementptr inbounds i32, i32* %ptr1, i64 1
+  %val1 = load i32, i32* %ptr1_plus1, align 4
+  store i32 0, i32* %ptr1, align 4
+  store i32 0, i32* %ptr2, align 4
+  ret i32 %val1
+}