Fix instruction counting in post-RA scheduler

[rampitec]

Summary

That is possible to bundle instructions but have no leading
BUNDLE. In such a case post-RA scheduler miscalculates a number
of instructions and asserts. The bundle in the test was created
by the Greedy.

[arsenm]

Is this really a well formed bundle? i.e. is this really a verifier bug?

[rampitec]

In D85456#2200529, @arsenm wrote:

Is this really a well formed bundle? i.e. is this really a verifier bug?

That's a good question. Technically that is possible to create such IR. It might be OK post-RA?
For the context, it was created by the SplitEditor::buildSingleSubRegCopy() in the D30438.

[arsenm]

I think this is an invalid bundle.

In D85456#2200548, @rampitec wrote:

In D85456#2200529, @arsenm wrote:

Is this really a well formed bundle? i.e. is this really a verifier bug?

That's a good question. Technically that is possible to create such IR. It might be OK post-RA?
For the context, it was created by the SplitEditor::buildSingleSubRegCopy() in the D30438.

I'm inclined to think it's invalid. It looks like using bundles at all was a hack. I think VirtRegRewriter::expandCopyBundle just missed the case where a copy becomes a kill.

Can you add a second test where this weird kill bundle thing is produced?

[rampitec]

Added test resulting in KILL bundle.

[rampitec]

In D85456#2200952, @arsenm wrote:

I think this is an invalid bundle.

In D85456#2200548, @rampitec wrote:

In D85456#2200529, @arsenm wrote:

Is this really a well formed bundle? i.e. is this really a verifier bug?

That's a good question. Technically that is possible to create such IR. It might be OK post-RA?
For the context, it was created by the SplitEditor::buildSingleSubRegCopy() in the D30438.

I'm inclined to think it's invalid. It looks like using bundles at all was a hack. I think VirtRegRewriter::expandCopyBundle just missed the case where a copy becomes a kill.

Can you add a second test where this weird kill bundle thing is produced?

Added splitkit-copy-bundle.mir. It produces a bundle with KILLs.

What's interesting I have created a patch to properly bundle copies in the SplitKit, which has resulted in KILLs surviving until inst printer and crash.

[kerbowa]

Seems wrong to have a bundle without BUNDLE opcode but the change seems fine obviously. Since it's allowed we should probably just account for the possibility.

[arsenm]

In D85456#2201120, @kerbowa wrote:

Seems wrong to have a bundle without BUNDLE opcode but the change seems fine obviously. Since it's allowed we should probably just account for the possibility.

But is it? I think this is a failing of the verifier and it is not allowed

[rampitec]

In D85456#2201140, @arsenm wrote:

In D85456#2201120, @kerbowa wrote:

Seems wrong to have a bundle without BUNDLE opcode but the change seems fine obviously. Since it's allowed we should probably just account for the possibility.

But is it? I think this is a failing of the verifier and it is not allowed

It probably isn't. But there seems to be quite a few places which do not work with properly formed bundles. One of them is ExpandPostRA.

[rampitec]

In D85456#2201163, @rampitec wrote:

In D85456#2201140, @arsenm wrote:

In D85456#2201120, @kerbowa wrote:

Seems wrong to have a bundle without BUNDLE opcode but the change seems fine obviously. Since it's allowed we should probably just account for the possibility.

But is it? I think this is a failing of the verifier and it is not allowed

It probably isn't. But there seems to be quite a few places which do not work with properly formed bundles. One of them is ExpandPostRA.

As weird as it sounds VirtRegRewriter::expandCopyBundle() does not work with properly formed bundles as well...

[rampitec]

Here is the alternative: D85484. It properly forms bundles in the SplitKit.

[rampitec]

Abandon in favor of D85484.

llvm/lib/CodeGen/PostRASchedulerList.cpp

for (MachineBasicBlock::iterator I = Current; I != MBB.begin();) {
         Scheduler.schedule();
         Scheduler.exitRegion();
         Scheduler.EmitSchedule();
         Current = &MI;
         CurrentCount = Count;
         Scheduler.Observe(MI, CurrentCount);
       }
       I = MI;
-      if (MI.isBundle())
+      if (MI.isBundledWithSucc())
         Count -= MI.getBundleSize();
     }
     assert(Count == 0 && "Instruction count mismatch!");
     assert((MBB.begin() == Current || CurrentCount != 0) &&
            "Instruction count mismatch!");
     Scheduler.enterRegion(&MBB, MBB.begin(), Current, CurrentCount);
     Scheduler.setEndIndex(CurrentCount);
     Scheduler.schedule();

llvm/test/CodeGen/AMDGPU/bundled_insts_no_bundle.mir

+# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=post-RA-sched -verify-machineinstrs -o - %s | FileCheck -check-prefix=GCN %s
+
+---
+# GCN-LABEL: name: bundled_insts_no_bundle
+# GCN:      entry:
+# GCN-NEXT:   $sgpr0 = KILL undef $sgpr0 {
+# GCN-NEXT:     $sgpr1 = KILL undef $sgpr1
+# GCN-NEXT:   }
+
+name: bundled_insts_no_bundle
+body:             |
+  bb.0.entry:
+
+    $sgpr0 = KILL undef $sgpr0 {
+      $sgpr1 = KILL undef $sgpr1
+    }
+
+...