[MachineScheduler] Add option to control reordering for store/load clustering #75338
Conversation
…ustering Reordering based on the sort order of the MemOpInfo array was disabled in <https://reviews.llvm.org/D72706>. However, it's not clear this is desirable for al targets. It also makes it more difficult to compare the incremental benefit of enabling load clustering in the selectiondag scheduler as well was the machinescheduler, as the sdag scheduler does seem to allow this reordering. This patch adds a parameter that can control the behaviour on a per-target basis.
|
@llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesReordering based on the sort order of the MemOpInfo array was disabled in <https://reviews.llvm.org/D72706>. However, it's not clear this is desirable for al targets. It also makes it more difficult to compare the incremental benefit of enabling load clustering in the selectiondag scheduler as well was the machinescheduler, as the sdag scheduler does seem to allow this reordering. This patch adds a parameter that can control the behaviour on a per-target basis. Split out from #73789. Full diff: https://github.com/llvm/llvm-project/pull/75338.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/MachineScheduler.h b/llvm/include/llvm/CodeGen/MachineScheduler.h
index 9f16cf5d5bc387..47127a6c29603b 100644
--- a/llvm/include/llvm/CodeGen/MachineScheduler.h
+++ b/llvm/include/llvm/CodeGen/MachineScheduler.h
@@ -1347,13 +1347,19 @@ ScheduleDAGMILive *createGenericSchedLive(MachineSchedContext *C);
/// Create a generic scheduler with no vreg liveness or DAG mutation passes.
ScheduleDAGMI *createGenericSchedPostRA(MachineSchedContext *C);
+/// If ReorderWhileClustering is set to true, no attempt will be made to
+/// reduce reordering due to store clustering.
std::unique_ptr<ScheduleDAGMutation>
createLoadClusterDAGMutation(const TargetInstrInfo *TII,
- const TargetRegisterInfo *TRI);
+ const TargetRegisterInfo *TRI,
+ bool ReorderWhileClustering = false);
+/// If ReorderWhileClustering is set to true, no attempt will be made to
+/// reduce reordering due to store clustering.
std::unique_ptr<ScheduleDAGMutation>
createStoreClusterDAGMutation(const TargetInstrInfo *TII,
- const TargetRegisterInfo *TRI);
+ const TargetRegisterInfo *TRI,
+ bool ReorderWhileClustering = false);
std::unique_ptr<ScheduleDAGMutation>
createCopyConstrainDAGMutation(const TargetInstrInfo *TII,
diff --git a/llvm/lib/CodeGen/MachineScheduler.cpp b/llvm/lib/CodeGen/MachineScheduler.cpp
index 886137d86f87de..554776783eff6f 100644
--- a/llvm/lib/CodeGen/MachineScheduler.cpp
+++ b/llvm/lib/CodeGen/MachineScheduler.cpp
@@ -1743,11 +1743,14 @@ class BaseMemOpClusterMutation : public ScheduleDAGMutation {
const TargetInstrInfo *TII;
const TargetRegisterInfo *TRI;
bool IsLoad;
+ bool ReorderWhileClustering;
public:
BaseMemOpClusterMutation(const TargetInstrInfo *tii,
- const TargetRegisterInfo *tri, bool IsLoad)
- : TII(tii), TRI(tri), IsLoad(IsLoad) {}
+ const TargetRegisterInfo *tri, bool IsLoad,
+ bool ReorderWhileClustering)
+ : TII(tii), TRI(tri), IsLoad(IsLoad),
+ ReorderWhileClustering(ReorderWhileClustering) {}
void apply(ScheduleDAGInstrs *DAGInstrs) override;
@@ -1763,14 +1766,16 @@ class BaseMemOpClusterMutation : public ScheduleDAGMutation {
class StoreClusterMutation : public BaseMemOpClusterMutation {
public:
StoreClusterMutation(const TargetInstrInfo *tii,
- const TargetRegisterInfo *tri)
- : BaseMemOpClusterMutation(tii, tri, false) {}
+ const TargetRegisterInfo *tri,
+ bool ReorderWhileClustering)
+ : BaseMemOpClusterMutation(tii, tri, false, ReorderWhileClustering) {}
};
class LoadClusterMutation : public BaseMemOpClusterMutation {
public:
- LoadClusterMutation(const TargetInstrInfo *tii, const TargetRegisterInfo *tri)
- : BaseMemOpClusterMutation(tii, tri, true) {}
+ LoadClusterMutation(const TargetInstrInfo *tii, const TargetRegisterInfo *tri,
+ bool ReorderWhileClustering)
+ : BaseMemOpClusterMutation(tii, tri, true, ReorderWhileClustering) {}
};
} // end anonymous namespace
@@ -1779,15 +1784,19 @@ namespace llvm {
std::unique_ptr<ScheduleDAGMutation>
createLoadClusterDAGMutation(const TargetInstrInfo *TII,
- const TargetRegisterInfo *TRI) {
- return EnableMemOpCluster ? std::make_unique<LoadClusterMutation>(TII, TRI)
+ const TargetRegisterInfo *TRI,
+ bool ReorderWhileClustering) {
+ return EnableMemOpCluster ? std::make_unique<LoadClusterMutation>(
+ TII, TRI, ReorderWhileClustering)
: nullptr;
}
std::unique_ptr<ScheduleDAGMutation>
createStoreClusterDAGMutation(const TargetInstrInfo *TII,
- const TargetRegisterInfo *TRI) {
- return EnableMemOpCluster ? std::make_unique<StoreClusterMutation>(TII, TRI)
+ const TargetRegisterInfo *TRI,
+ bool ReorderWhileClustering) {
+ return EnableMemOpCluster ? std::make_unique<StoreClusterMutation>(
+ TII, TRI, ReorderWhileClustering)
: nullptr;
}
@@ -1840,7 +1849,7 @@ void BaseMemOpClusterMutation::clusterNeighboringMemOps(
SUnit *SUa = MemOpa.SU;
SUnit *SUb = MemOpb.SU;
- if (SUa->NodeNum > SUb->NodeNum)
+ if (!ReorderWhileClustering && SUa->NodeNum > SUb->NodeNum)
std::swap(SUa, SUb);
// FIXME: Is this check really required?
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index 3abdb6003659fa..ae10a72e889239 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -348,7 +348,7 @@ class RISCVPassConfig : public TargetPassConfig {
ScheduleDAGMILive *DAG = nullptr;
if (EnableMISchedLoadClustering) {
DAG = createGenericSchedLive(C);
- DAG->addMutation(createLoadClusterDAGMutation(DAG->TII, DAG->TRI));
+ DAG->addMutation(createLoadClusterDAGMutation(DAG->TII, DAG->TRI, true));
}
if (ST.hasMacroFusion()) {
DAG = DAG ? DAG : createGenericSchedLive(C);
diff --git a/llvm/test/CodeGen/RISCV/misched-load-clustering.ll b/llvm/test/CodeGen/RISCV/misched-load-clustering.ll
index 4eb969a357a9ee..d046aaf98f5edd 100644
--- a/llvm/test/CodeGen/RISCV/misched-load-clustering.ll
+++ b/llvm/test/CodeGen/RISCV/misched-load-clustering.ll
@@ -23,10 +23,10 @@ define i32 @load_clustering_1(ptr nocapture %p) {
; LDCLUSTER: ********** MI Scheduling **********
; LDCLUSTER-LABEL: load_clustering_1:%bb.0
; LDCLUSTER: *** Final schedule for %bb.0 ***
-; LDCLUSTER: SU(5): %6:gpr = LW %0:gpr, 16
-; LDCLUSTER: SU(1): %1:gpr = LW %0:gpr, 12
-; LDCLUSTER: SU(2): %2:gpr = LW %0:gpr, 8
; LDCLUSTER: SU(4): %4:gpr = LW %0:gpr, 4
+; LDCLUSTER: SU(2): %2:gpr = LW %0:gpr, 8
+; LDCLUSTER: SU(1): %1:gpr = LW %0:gpr, 12
+; LDCLUSTER: SU(5): %6:gpr = LW %0:gpr, 16
entry:
%arrayidx0 = getelementptr inbounds i32, ptr %p, i32 3
%val0 = load i32, i32* %arrayidx0
|
|
Ping. |
|
Are you saying that reordering the instructions to match the order in MemOpInfo is positively desirable in some cases? I only disabled it because it seemed completely arbitrary to me. I think it would be even better if we could find some way to schedule the instructions adjacently without enforcing any particular order, but I don't see any way to do this just by adding edges to the DAG. |
It's definitely somewhat arbitrary. I don't see it being harmful though, and it's attractive to me that the same sorting is used for clustering in the MachineScheduler as for the SelectionDAG (which also sorts in increasing order of offset in ScheduleDAGSDNodes::ClusterNeighbouringLoads). As a reader of the generated assembly, if the order of neighbouring loads is unimportant then I also have a slight preference for them being in sorted order as that's clearly arbitrary, while unsorted has the false appearance of being meaningful. I'd say matching the SDag scheduler is the main one that motivated bothering to patch this - having the same order makes it easier to compare what missed opportunities / differences there are when enabling load clustering there as well. |
I did not know about ScheduleDAGSDNodes::ClusterNeighbouringLoads. I don't have any objection to this patch. |
|
Ping. |
…ustering (llvm#75338) Reordering based on the sort order of the MemOpInfo array was disabled in <https://reviews.llvm.org/D72706>. However, it's not clear this is desirable for al targets. It also makes it more difficult to compare the incremental benefit of enabling load clustering in the selectiondag scheduler as well was the machinescheduler, as the sdag scheduler does seem to allow this reordering. This patch adds a parameter that can control the behaviour on a per-target basis. Split out from llvm#73789.
Reordering based on the sort order of the MemOpInfo array was disabled in https://reviews.llvm.org/D72706. However, it's not clear this is desirable for al targets. It also makes it more difficult to compare the incremental benefit of enabling load clustering in the selectiondag scheduler as well was the machinescheduler, as the sdag scheduler does seem to allow this reordering.
This patch adds a parameter that can control the behaviour on a per-target basis.
Split out from #73789.