[SystemZ] Don't crash with -misched-cutoff

[jonpa]

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=45928, which reports that SystemZPostRASchedStrategy crashes with -mished-cutoff.

The reason seems to be that dangling pointers in the Available set were never cleared.

This patch clears the set if needed and then also skips advancing the HazardRecognizer through the unscheduled instructions.

[uweigand]

Could you elaborate why initPolicy is the correct place to clear the Available list? I'm wondering because the default implementation doesn't appear to do that either, it looks like common code only clears the list in the main "init" ...

[jonpa]

In D94383#2489954, @uweigand wrote:

Could you elaborate why initPolicy is the correct place to clear the Available list? I'm wondering because the default implementation doesn't appear to do that either, it looks like common code only clears the list in the main "init" ...

My main concern was that it is cleared before each region, and it seems that either SystemZPostRASchedStrategy::initialize() and SystemZPostRASchedStrategy::initPolicy() could work.

For a region containing just a single instruction (scheduling boundary), only SystemZPostRASchedStrategy::initPolicy() is called, which is why that method is used to update the hazard recognizer with those instructions as well so that it is accurate when the next region begins with actual scheduling.

My idea - based on the assumption that this is a compile-time issue - was to clear Available and also skip updating the HazardRecognizer in cases where the scheduling had reached the limit. I thought that since pickNode() is called for the first instruction in the region, that the cutoff happens for each region, but that was wrong - as soon as the cutoff has been reached, all scheduling stops. I don't understand really why the DAG is built and pickNode() is called for each region when no scheduling will occur...

I think we could probably only clear Available in either initPolicy() or initialize() - the cost of advancing the hazard recognizer should be much less than building the DAGs...?

[uweigand]

I don't think we need to bother with skipping advancing the hazard state. I believe the main point of the cutoff is to avoid combinatorial explosion where there are many instructions to schedule and at each step there are many candidates to consider. Advancing the hazard state doesn't consider candidates and is therefore just a linear pass over instructions.

I'd simply clear the Available list once in ::initalize. That's where other MachineSchedStrategy implementations also clear their respective queues.

[jonpa]

Patch updated per review.

The test "crashes now" without this patch, but I guess there is no guarantee that it is meaningful in the long-run given that it triggers asserts when accessing stale SU pointers...

[uweigand]

LGTM

llvm/lib/Target/SystemZ/SystemZMachineScheduler.cpp

advanceTo(MachineBasicBlock::iterator NextBegin) {
   for (; I != NextBegin; ++I) {
     if (I->isPosition() || I->isDebugInstr())
       continue;
     HazardRec->emitInstruction(&*I);
   }
 }
 
 void SystemZPostRASchedStrategy::initialize(ScheduleDAGMI *dag) {
+  Available.clear();  // -misched-cutoff.
   LLVM_DEBUG(HazardRec->dumpState(););
 }
 
 void SystemZPostRASchedStrategy::enterMBB(MachineBasicBlock *NextMBB) {
   assert ((SchedStates.find(NextMBB) == SchedStates.end()) &&
           "Entering MBB twice?");
   LLVM_DEBUG(dbgs() << "** Entering " << printMBBReference(*NextMBB));
 

llvm/test/CodeGen/SystemZ/misched-cutoff.ll

+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -misched-cutoff=1 -o /dev/null < %s
+;
+; Test that the post-ra scheduler does not crash with -misched-cutoff.
+
+@g_184 = external dso_local global i16, align 2
+@g_294 = external dso_local global [1 x [9 x i32*]], align 8
+
+define void @fun() {
+bb:
+  br label %bb1
+
+bb1:                                              ; preds = %bb1, %bb
+  %i = phi i64 [ 0, %bb ], [ %i22, %bb1 ]
+  %i2 = trunc i64 %i to i32
+  %i3 = lshr i32 %i2, 1
+  %i4 = select i1 false, i32 %i3, i32 undef
+  %i5 = lshr i32 %i4, 1
+  %i6 = xor i32 %i5, -306674912
+  %i7 = select i1 undef, i32 %i5, i32 %i6
+  %i8 = lshr i32 %i7, 1
+  %i9 = xor i32 %i8, -306674912
+  %i10 = select i1 undef, i32 %i8, i32 %i9
+  %i11 = lshr i32 %i10, 1
+  %i12 = xor i32 %i11, -306674912
+  %i13 = select i1 undef, i32 %i11, i32 %i12
+  %i14 = lshr i32 %i13, 1
+  %i15 = select i1 false, i32 %i14, i32 undef
+  %i16 = lshr i32 %i15, 1
+  %i17 = select i1 false, i32 %i16, i32 undef
+  %i18 = lshr i32 %i17, 1
+  %i19 = select i1 false, i32 %i18, i32 undef
+  %i20 = lshr i32 %i19, 1
+  %i21 = select i1 false, i32 %i20, i32 undef
+  store i32 %i21, i32* undef, align 4
+  %i22 = add nuw nsw i64 %i, 1
+  %i23 = icmp ult i64 %i, 255
+  br i1 %i23, label %bb1, label %bb24
+
+bb24:                                             ; preds = %bb1
+  %i25 = load volatile i16, i16* undef
+  store i32* null, i32** undef, align 8
+  store i32 -10, i32* undef, align 4
+  store i32 -10, i32* null, align 4
+  store i32 -10, i32* undef, align 4
+  store i16 0, i16* @g_184, align 2
+  store i32* null, i32** getelementptr inbounds ([1 x [9 x i32*]], [1 x [9 x i32*]]* @g_294, i64 0, i64 0, i64 2), align 8
+  store i32* null, i32** getelementptr inbounds ([1 x [9 x i32*]], [1 x [9 x i32*]]* @g_294, i64 0, i64 0, i64 5), align 8
+  unreachable
+}