[RISCV][MISched] Set EnableIntervals to true for SiFive7 #75681
Conversation
The SiFive7 scheduler model has been using AcquireAtCycles and ReleaseAtCycles for some time. Without EnableIntervals, the scheduler was not making decisions based on this information. This patch sets EnableIntervals to true, and the test case demonstrates that the VADD instructions can be issued one cycle earlier since the VCQ is not reserved. This leads to better saturation of the VA. I wonder whether it would be better (in a future patch) to move EnableIntervals out of the scheduler model and add an option like `use-intervals-to-schedule` so that we can control per-program basis. EnableIntervals isn't really a property of the scheduler model itself, and more a property of how the scheduler behaves. FWIW, I had no idea EnableIntervals existed, and when I added AcquireAtCycles and ReleaseAtCycles, I totally assumed that the scheduler had been using this information out of the box.
|
@llvm/pr-subscribers-backend-risc-v Author: Michael Maitland (michaelmaitland) ChangesThe SiFive7 scheduler model has been using AcquireAtCycles and ReleaseAtCycles for some time. Without EnableIntervals, the scheduler was not making decisions based on this information. This patch sets EnableIntervals to true, and the test case demonstrates that the VADD instructions can be issued one cycle earlier since the VCQ is not reserved. This leads to better saturation of the VA. I wonder whether it would be better (in a future patch) to move EnableIntervals out of the scheduler model and add an option like FWIW, I had no idea EnableIntervals existed, and when I added AcquireAtCycles and ReleaseAtCycles, I totally assumed that the scheduler had been using this information out of the box. Full diff: https://github.com/llvm/llvm-project/pull/75681.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td b/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
index 45783d482f3bd7..f531ab2fac8f9f 100644
--- a/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
+++ b/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
@@ -198,6 +198,7 @@ def SiFive7Model : SchedMachineModel {
let LoadLatency = 3;
let MispredictPenalty = 3;
let CompleteModel = 0;
+ let EnableIntervals = true;
let UnsupportedFeatures = [HasStdExtZbkb, HasStdExtZbkc, HasStdExtZbkx,
HasStdExtZcmt, HasStdExtZknd, HasStdExtZkne,
HasStdExtZknh, HasStdExtZksed, HasStdExtZksh,
diff --git a/llvm/test/CodeGen/RISCV/sifive7-enable-intervals.mir b/llvm/test/CodeGen/RISCV/sifive7-enable-intervals.mir
new file mode 100644
index 00000000000000..20bebc4d2b5f8b
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/sifive7-enable-intervals.mir
@@ -0,0 +1,56 @@
+# RUN: llc -mtriple=riscv64 -mcpu=sifive-x280 -run-pass=machine-scheduler \
+# RUN: -debug-only=machine-scheduler -misched-dump-schedule-trace \
+# RUN: -misched-topdown -o - %s 2>&1 | FileCheck %s
+
+# The purpose of this test is to show that the VADD instructions are issued at
+# so that the SiFive7VA is saturated.
+---
+name: add_m2
+tracksRegLiveness: true
+body: |
+ bb.0.entry:
+ liveins: $v8m2, $v10m2, $v12m2, $v14m2, $x10
+
+ %4:gprnox0 = COPY $x10
+ %3:vrm2 = COPY $v14m2
+ %2:vrm2 = COPY $v12m2
+ %1:vrm2 = COPY $v10m2
+ %0:vrm2 = COPY $v8m2
+ dead $x0 = PseudoVSETVLI %4, 217 /* e64, m2, ta, ma */, implicit-def $vl, implicit-def $vtype
+ %5:vrm2 = PseudoVADD_VV_M2 undef %5, %0, %1, $noreg, 6 /* e64 */, 0 /* tu, mu */, implicit $vl, implicit $vtype
+ %6:vrm2 = PseudoVADD_VV_M2 undef %6, %3, %2, $noreg, 6 /* e64 */, 0 /* tu, mu */, implicit $vl, implicit $vtype
+ %7:vrm2 = PseudoVADD_VV_M2 undef %7, %5, %6, $noreg, 6 /* e64 */, 0 /* tu, mu */, implicit $vl, implicit $vtype
+ $v8m2 = COPY %7
+ PseudoRET implicit $v8m2
+
+# CHECK: *** Final schedule for %bb.0 ***
+# CHECK-NEXT: * Schedule table (TopDown):
+# CHECK-NEXT: i: issue
+# CHECK-NEXT: x: resource booked
+# CHECK-NEXT: Cycle | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 |
+# CHECK-NEXT: SU(0) | i | | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7PipeAB | x | | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SU(1) | i | | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7PipeAB | x | | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SU(2) | | i | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7PipeAB | | x | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SU(3) | | i | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7PipeAB | | x | | | | | | | | | | | | | | | |
+# CHECK-NEXT: SU(4) | | | i | | | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7PipeAB | | | x | | | | | | | | | | | | | | |
+# CHECK-NEXT: SU(5) | | | i | | | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7PipeA | | | x | | | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7PipeAB | | | x | | | | | | | | | | | | | | |
+# CHECK-NEXT: SU(7) | | | | | i | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7VCQ | | | | | x | | | | | | | | | | | | |
+# CHECK-NEXT: SiFive7VA | | | | | | x | x | x | x | | | | | | | | |
+# CHECK-NEXT: SU(6) | | | | | | | | | i | | | | | | | | |
+# CHECK-NEXT: SiFive7VCQ | | | | | | | | | x | | | | | | | | |
+# CHECK-NEXT: SiFive7VA | | | | | | | | | | x | x | x | x | | | | |
+# CHECK-NEXT: SU(8) | | | | | | | | | | | | | i | | | | |
+# CHECK-NEXT: SiFive7VCQ | | | | | | | | | | | | | x | | | | |
+# CHECK-NEXT: SiFive7VA | | | | | | | | | | | | | | x | x | x | x |
+# CHECK-NEXT: SU(9) | | | | | | | | | | | | | | | | | i |
+# CHECK-NEXT: SiFive7PipeAB | | | | | | | | | | | | | | | | | x |
+
+...
|
Please create an issue, we will discuss it there. Thank you! |
|
Does it fail in -Asserts? |
I haven’t seen any failures. Are you asking for any specific reason? |
|
We're seeing the same failure as shown in this link. |
|
We're seeing the same test failure on our builders, can we revert this change? |
|
We are also seeing this failure on our internal release build with assertions disabled. |
|
I think this needs |
|
For now, I think we should revert. What's the process for that now that we're on PRs? PR for the revert? |
|
@nikic thanks for the fix. Sorry to all who had broken builds. I did not see your messages until I woke up. |
The SiFive7 scheduler model has been using AcquireAtCycles and ReleaseAtCycles for some time. Without EnableIntervals, the scheduler was not making decisions based on this information. This patch sets EnableIntervals to true, and the test case demonstrates that the VADD instructions can be issued one cycle earlier since the VCQ is not reserved. This leads to better saturation of the VA.
I wonder whether it would be better (in a future patch) to move EnableIntervals out of the scheduler model and add an option like
use-intervals-to-scheduleso that we can control per-program basis. EnableIntervals isn't really a property of the scheduler model itself, and more a property of how the scheduler behaves.FWIW, I had no idea EnableIntervals existed, and when I added AcquireAtCycles and ReleaseAtCycles, I totally assumed that the scheduler had been using this information out of the box.