[RISCV] Add load/store clustering in post machine schedule #111504
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
wangpc-pp
requested review from
lukel97,
asb,
topperc,
wangpc-pp,
michaelmaitland and
dtcxzyw
Hi, I'm struggling to parse this a bit. I think you're saying that if load and store clustering aren't added to the post machine schedule in addition to being in If you could update this PR to show any test differences that would really help with review. Thanks! |
|
FWIW, if I add a version of this on top of #73796 I get assertions in several llvm/test/CodeGen/RISCV tests EDIT: SNIP - thanks wangpc-pp, the problem was indeed on my end! |
I think the reason is you are using pre-ra scheduler here. |
Thank you - the problem was indeed me being too eager with copy and paste and failing to switch to |
Thanks! I will ask @BoyaoWang430 to add some MIR tests. And thanks in advance if you can help to evaluate/review this PR. :-) |
Thanks! And more generally, any note you have on how/if it affects codegen on external codebases very welcome. e.g. is this something that kicks in a lot in real-world code but we just don't trigger in our tests, or is it fairly rare it makes a difference (but of course worth addressing for the cases it helps) |
We also made the same mistake when we first tried to add this by copy-paste. I will add some MIR tests these days to demonstrate the difference that enabling load/store clustering in the post machine scheduler can make. |
The reason why there is no CodeGen change in in-tree tests is because postra scheduler is not enabled by default. To enable postra scheduler, we should use a CPU with The problem is very common, maybe I didn't make it clear in sync-up meeting because of my bad speaking :-(.
# Before pre-ra scheduling
ld vreg0, 0(rs)
addi vreg1, vreg0, 1
ld vreg2, 8(rs)
addi vreg3, vreg2, 1
# After pre-ra scheduling
ld vreg0, 0(rs)
ld vreg2, 8(rs)
addi vreg1, vreg0, 1
addi vreg3, vreg2, 1
# After post-ra scheduling (possile current result)
ld a1, 0(a0)
addi a3, a1, 1
ld a2, 8(a0)
addi a4, a2, 1
# After post-ra scheduling (what we want)
ld a1, 0(a0)
ld a2, 8(a0)
addi a3, a1, 1
addi a4, a2, 1I think this problem is common across targets and as what you have said PPC have already done it. |
|
Why is this what we want? What is problem if there is an add between them? Is there some fusion going on in the hardware? I understand why you want them loads ordered near each other if they access the same cache line, but I don't understand why arithmetic between them is bad? |
Oh, sorry, my example may not be exact. Several reasons:
Anyway, we saw the benefit and if it is not common for all μ-arch, we can make it a feature. |
|
@llvm/pr-subscribers-backend-risc-v Author: None (BoyaoWang430) Changes#73789 added load clustering and #73796 tried to add store clustering. Full diff: https://github.com/llvm/llvm-project/pull/111504.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index 089dc6c529193d..4c8faed5a4787a 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -363,6 +363,19 @@ class RISCVPassConfig : public TargetPassConfig {
return DAG;
}
+ ScheduleDAGInstrs *
+ createPostMachineScheduler(MachineSchedContext *C) const override {
+ ScheduleDAGMI *DAG = nullptr;
+ if (EnableMISchedLoadStoreClustering) {
+ DAG = createGenericSchedPostRA(C);
+ DAG->addMutation(createLoadClusterDAGMutation(
+ DAG->TII, DAG->TRI, /*ReorderWhileClustering=*/true));
+ DAG->addMutation(createStoreClusterDAGMutation(
+ DAG->TII, DAG->TRI, /*ReorderWhileClustering=*/true));
+ }
+ return DAG;
+ }
+
void addIRPasses() override;
bool addPreISel() override;
void addCodeGenPrepare() override;
diff --git a/llvm/test/CodeGen/RISCV/misched-mem-clustering.mir b/llvm/test/CodeGen/RISCV/misched-mem-clustering.mir
new file mode 100644
index 00000000000000..4764e08969da5d
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/misched-mem-clustering.mir
@@ -0,0 +1,64 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=riscv64 -x mir -mcpu=sifive-p470 -mattr=+use-postra-scheduler -verify-misched -enable-post-misched=true \
+# RUN: -riscv-misched-load-store-clustering=false -debug-only=machine-scheduler \
+# RUN: -start-before=machine-scheduler -stop-after=postmisched -o - 2>&1 < %s \
+# RUN: | FileCheck -check-prefix=NOCLUSTER %s
+# RUN: llc -mtriple=riscv64 -x mir -mcpu=sifive-p470 -mattr=+use-postra-scheduler -verify-misched -enable-post-misched=true \
+# RUN: -debug-only=machine-scheduler \
+# RUN: -start-before=machine-scheduler -stop-after=postmisched -o - 2>&1 < %s \
+# RUN: | FileCheck -check-prefix=MEMCLUSTER %s
+
+...
+---
+name: mem_clustering_1
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $x6, $x10, $x14, $x15, $x16, $x17
+ ; NOCLUSTER-LABEL: name: mem_clustering_1
+ ; NOCLUSTER: liveins: $x6, $x10, $x14, $x15, $x16, $x17
+ ; NOCLUSTER-NEXT: {{ $}}
+ ; NOCLUSTER-NEXT: renamable $x5 = LW renamable $x15, 0 :: (load (s32))
+ ; NOCLUSTER-NEXT: SW renamable $x14, renamable $x15, 0 :: (store (s32))
+ ; NOCLUSTER-NEXT: renamable $x11 = ADDW killed renamable $x6, killed renamable $x5
+ ; NOCLUSTER-NEXT: renamable $x7 = LW renamable $x15, 8 :: (load (s32))
+ ; NOCLUSTER-NEXT: renamable $x28 = LW renamable $x15, 16 :: (load (s32))
+ ; NOCLUSTER-NEXT: renamable $x29 = LW renamable $x15, 24 :: (load (s32))
+ ; NOCLUSTER-NEXT: renamable $x13 = ADDW killed renamable $x7, killed renamable $x28
+ ; NOCLUSTER-NEXT: SW renamable $x14, renamable $x15, 8 :: (store (s32))
+ ; NOCLUSTER-NEXT: SW renamable $x14, renamable $x15, 16 :: (store (s32))
+ ; NOCLUSTER-NEXT: SW killed renamable $x14, killed renamable $x15, 24 :: (store (s32))
+ ; NOCLUSTER-NEXT: renamable $x11 = ADDW killed renamable $x11, killed renamable $x13
+ ; NOCLUSTER-NEXT: renamable $x6 = ADDW killed renamable $x11, killed renamable $x29
+ ; NOCLUSTER-NEXT: PseudoRET
+ ;
+ ; MEMCLUSTER-LABEL: name: mem_clustering_1
+ ; MEMCLUSTER: liveins: $x6, $x10, $x14, $x15, $x16, $x17
+ ; MEMCLUSTER-NEXT: {{ $}}
+ ; MEMCLUSTER-NEXT: renamable $x5 = LW renamable $x15, 0 :: (load (s32))
+ ; MEMCLUSTER-NEXT: renamable $x7 = LW renamable $x15, 8 :: (load (s32))
+ ; MEMCLUSTER-NEXT: renamable $x28 = LW renamable $x15, 16 :: (load (s32))
+ ; MEMCLUSTER-NEXT: renamable $x29 = LW renamable $x15, 24 :: (load (s32))
+ ; MEMCLUSTER-NEXT: SW renamable $x14, renamable $x15, 0 :: (store (s32))
+ ; MEMCLUSTER-NEXT: SW renamable $x14, renamable $x15, 8 :: (store (s32))
+ ; MEMCLUSTER-NEXT: SW renamable $x14, renamable $x15, 16 :: (store (s32))
+ ; MEMCLUSTER-NEXT: SW killed renamable $x14, killed renamable $x15, 24 :: (store (s32))
+ ; MEMCLUSTER-NEXT: renamable $x11 = ADDW killed renamable $x6, killed renamable $x5
+ ; MEMCLUSTER-NEXT: renamable $x13 = ADDW killed renamable $x7, killed renamable $x28
+ ; MEMCLUSTER-NEXT: renamable $x11 = ADDW killed renamable $x11, killed renamable $x13
+ ; MEMCLUSTER-NEXT: renamable $x6 = ADDW killed renamable $x11, killed renamable $x29
+ ; MEMCLUSTER-NEXT: PseudoRET
+ renamable $x5 = LW renamable $x15, 0 :: (load (s32))
+ renamable $x7 = LW renamable $x15, 8 :: (load (s32))
+ renamable $x28 = LW renamable $x15, 16 :: (load (s32))
+ renamable $x29 = LW renamable $x15, 24 :: (load (s32))
+ SW renamable $x14, renamable $x15, 0 :: (store (s32))
+ SW renamable $x14, renamable $x15, 8 :: (store (s32))
+ SW renamable $x14, renamable $x15, 16 :: (store (s32))
+ SW renamable $x14, renamable $x15, 24 :: (store (s32))
+ renamable $x11 = ADDW killed renamable $x6, killed renamable $x5
+ renamable $x13 = ADDW killed renamable $x7, killed renamable $x28
+ renamable $x11 = ADDW killed renamable $x11, killed renamable $x13
+ renamable $x6 = ADDW killed renamable $x11, killed renamable $x29
+ PseudoRET
+...
|
| ScheduleDAGInstrs * | ||
| createPostMachineScheduler(MachineSchedContext *C) const override { | ||
| ScheduleDAGMI *DAG = nullptr; | ||
| if (EnableMISchedLoadStoreClustering) { |
There was a problem hiding this comment.
Can we add a new debug option to control if we should enable postra clustering?
static cl::opt<bool> EnablePostMISchedLoadStoreClustering(
"riscv-postmisched-load-store-clustering", cl::Hidden,
cl::desc("Enable PostRA load and store clustering in the machine scheduler"),
cl::init(true));
if (EnableMISchedLoadStoreClustering && EnablePostMISchedLoadStoreClustering) {
...
}
|
Ping. |
If post machine schedule is used, previous cluster of load/store which formed in machine schedule may break. In order to solve this, add load/sotre clustering to post machine schedule.
|
@BoyaoWang430 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/7711 Here is the relevant piece of the build log for the reference |
Should be fixed by 37ce189. |
#73789 added load clustering and #73796 tried to add store clustering.
If post machine schedule is used, previous cluster of load/store which formed in machine schedule may break. In order to solve this, add load/sotre clustering to post machine schedule.