[SystemZ] Enable liveness reduction in pre-RA sched strategy.#188823
Conversation
|
@llvm/pr-subscribers-backend-systemz Author: Jonas Paulsson (JonPsson1) ChangesRemove the "data-sequnces" check for latency reduction - this makes latency reduction happen in more regions. Add some handling of liveness reduction by means of scheduling an SU that closes a live range "low".
Conclusion: Just like last time this was tried without the data-sequences heuristic these numbers deteriorate somewhat. However, performance measurements indicate that this does not matter in the end - the handling of latencies and "top-of-region" seems to be beneficial in this regard despite these numbers. As a follow-up it may be of interest to try to either improve on the cmp-elim results (did not work last time), or perhaps remove that handling if it doesn't help much.
(Before rebase, 3dfeeff was NFC with 19c0077.) Patch is 189.52 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/188823.diff 48 Files Affected:
diff --git a/llvm/lib/Target/SystemZ/SystemZMachineScheduler.cpp b/llvm/lib/Target/SystemZ/SystemZMachineScheduler.cpp
index 8df9540d1a0e5..b75a0d8f7f901 100644
--- a/llvm/lib/Target/SystemZ/SystemZMachineScheduler.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZMachineScheduler.cpp
@@ -15,6 +15,18 @@ using namespace llvm;
/// Pre-RA scheduling ///
+// Options needed for testing.
+// TopRegionSUs is the number of SUs that are considered to be part of the
+// "top" of a region. Liveness reduction is not done at all in regions
+// smaller than this, and in bigger regions this is the number of high SUs
+// (by Height) that are not scheduled low to reduce liveness. The idea is to
+// prioritize latency more after branches and help liveness only when the
+// decoder is ahead of execution anyway. Handle a top region with this many
+// SUs (decoder cycles * 6).
+static cl::opt<unsigned> TopRegionSUs("top-region", cl::Hidden, cl::init(36));
+static cl::opt<bool> DisableLatency("disable-latency", cl::Hidden,
+ cl::init(false));
+
static bool isRegDef(const MachineOperand &MO) {
return MO.isReg() && MO.isDef();
}
@@ -23,53 +35,40 @@ static bool isPhysRegDef(const MachineOperand &MO) {
return isRegDef(MO) && MO.getReg().isPhysical();
}
+void SystemZPreRASchedStrategy::initializeLivenessReduction() {
+ HighSUs.clear();
+ if (!RegionPolicy.ShouldTrackPressure)
+ return;
+
+ std::vector<const SUnit *> SUs;
+ SUs.reserve(DAG->SUnits.size());
+ for (auto &Itr : DAG->SUnits)
+ SUs.push_back(&Itr);
+
+ std::sort(SUs.begin(), SUs.end(), [](const SUnit *lhs, const SUnit *rhs) {
+ if (lhs->getHeight() > rhs->getHeight())
+ return true;
+ else if (lhs->getHeight() < rhs->getHeight())
+ return false;
+ return lhs->NodeNum < rhs->NodeNum;
+ });
+
+ HighSUs.insert(SUs.begin(), SUs.begin() + TopRegionSUs);
+}
+
void SystemZPreRASchedStrategy::initializeLatencyReduction() {
- // Enable latency reduction for a region that has a considerable amount of
- // data sequences that should be interlaved. These are SUs that only have
- // one data predecessor / successor edge(s) to their adjacent instruction(s)
- // in the input order. Disable if region has many SUs relative to the
- // overall height.
+ if (DisableLatency) {
+ RegionPolicy.DisableLatencyHeuristic = true;
+ return;
+ }
+
+ // Disable latency reduction if region has many SUs relative to the overall
+ // height.
unsigned DAGHeight = 0;
for (unsigned Idx = 0, End = DAG->SUnits.size(); Idx != End; ++Idx)
DAGHeight = std::max(DAGHeight, DAG->SUnits[Idx].getHeight());
RegionPolicy.DisableLatencyHeuristic =
DAG->SUnits.size() >= 3 * std::max(DAGHeight, 1u);
- if ((HasDataSequences = !RegionPolicy.DisableLatencyHeuristic)) {
- unsigned CurrSequence = 0, NumSeqNodes = 0;
- auto countSequence = [&CurrSequence, &NumSeqNodes]() {
- if (CurrSequence >= 2)
- NumSeqNodes += CurrSequence;
- CurrSequence = 0;
- };
- for (unsigned Idx = 0, End = DAG->SUnits.size(); Idx != End; ++Idx) {
- const SUnit *SU = &DAG->SUnits[Idx];
- bool InDataSequence = true;
- // One Data pred to MI just above, or no preds.
- unsigned NumPreds = 0;
- for (const SDep &Pred : SU->Preds)
- if (++NumPreds != 1 || Pred.getKind() != SDep::Data ||
- Pred.getSUnit()->NodeNum != Idx - 1)
- InDataSequence = false;
- // One Data succ or no succs (ignoring ExitSU).
- unsigned NumSuccs = 0;
- for (const SDep &Succ : SU->Succs)
- if (Succ.getSUnit() != &DAG->ExitSU &&
- (++NumSuccs != 1 || Succ.getKind() != SDep::Data))
- InDataSequence = false;
- // Another type of node or one that does not have a single data pred
- // ends any previous sequence.
- if (!InDataSequence || !NumPreds)
- countSequence();
- if (InDataSequence)
- CurrSequence++;
- }
- countSequence();
- if (NumSeqNodes >= std::max(size_t(4), DAG->SUnits.size() / 4)) {
- LLVM_DEBUG(dbgs() << "Number of nodes in def-use sequences: "
- << NumSeqNodes << ". ";);
- } else
- HasDataSequences = false;
- }
}
bool SystemZPreRASchedStrategy::definesCmp0Src(const MachineInstr *MI,
@@ -83,6 +82,44 @@ bool SystemZPreRASchedStrategy::definesCmp0Src(const MachineInstr *MI,
return false;
}
+bool SystemZPreRASchedStrategy::closesLiveRange(const SUnit *SU,
+ ScheduleDAGMILive *DAG) const {
+ if (SU->getInstr()->isCopy())
+ return false;
+
+ // Extract the PressureChanges that all fp/vector or GR64/GR32/GRH32 regs
+ // affect respectively. misched-prera-pdiffs.mir tests against any future
+ // change in the PressureSets modelling, so simply hard-code them here.
+ int VR16PChange = 0, GRX32PChange = 0;
+ const PressureDiff &PDiff = DAG->getPressureDiff(SU);
+ for (const PressureChange &PC : PDiff) {
+ if (!PC.isValid())
+ break;
+ if (PC.getPSet() == SystemZ::VR16Bit)
+ VR16PChange = PC.getUnitInc();
+ else if (PC.getPSet() == SystemZ::GRX32Bit)
+ GRX32PChange = PC.getUnitInc();
+ }
+
+ // Return true for a (vreg) def when no uses become live. Prioritize
+ // FP/vector regs over GPRs.
+ const MachineOperand &MO0 = SU->getInstr()->getOperand(0);
+ if (isRegDef(MO0)) {
+ const TargetRegisterClass *RC = DAG->MRI.getRegClass(MO0.getReg());
+ int RegWeight = TRI->getRegClassWeight(RC).RegWeight;
+ bool VR16DefNoKill = VR16PChange == -RegWeight;
+ bool GRX32DefNoKill = GRX32PChange == -RegWeight;
+ return VR16DefNoKill || (!VR16PChange && GRX32DefNoKill);
+ }
+ return false;
+}
+
+unsigned SystemZPreRASchedStrategy::getRemLat(SchedBoundary *Zone) const {
+ if (RemLat == ~0U)
+ RemLat = computeRemLatency(*Zone);
+ return RemLat;
+}
+
static int biasPhysRegExtra(const SUnit *SU) {
if (int Res = biasPhysReg(SU, /*isTop=*/false))
return Res;
@@ -115,20 +152,26 @@ bool SystemZPreRASchedStrategy::tryCandidate(SchedCandidate &Cand,
return TryCand.Reason != NoCand;
}
- // Don't extend the scheduled latency in regions with many nodes in data
- // sequences, or for (single block loop) regions that are acyclically
- // (within a single loop iteration) latency limited. IsAcyclicLatencyLimited
- // is set only after initialization in registerRoots(), which is why it is
- // checked here instead of earlier.
- if (!RegionPolicy.DisableLatencyHeuristic &&
- (HasDataSequences || Rem.IsAcyclicLatencyLimited))
+ if (RegionPolicy.ShouldTrackPressure) {
+ auto schedLow = [&](const SUnit *SU) {
+ return SU->getHeight() <= Zone->getScheduledLatency() &&
+ !HighSUs.count(SU) && closesLiveRange(SU, DAG);
+ };
+ // One SU closes a live range while preserving the scheduled latency.
+ if (tryGreater(schedLow(TryCand.SU), schedLow(Cand.SU), TryCand, Cand,
+ RegExcess))
+ return TryCand.Reason != NoCand;
+ }
+
+ if (!RegionPolicy.DisableLatencyHeuristic)
if (const SUnit *HigherSU =
TryCand.SU->getHeight() > Cand.SU->getHeight() ? TryCand.SU
: TryCand.SU->getHeight() < Cand.SU->getHeight() ? Cand.SU
: nullptr)
if (HigherSU->getHeight() > Zone->getScheduledLatency() &&
- HigherSU->getDepth() < computeRemLatency(*Zone)) {
- // One or both SUs increase the scheduled latency.
+ HigherSU->getDepth() < getRemLat(Zone)) {
+ // The higher SU increases the scheduled latency but is not on the
+ // Critical Path by Depth, so put it above the other one.
tryLess(TryCand.SU->getHeight(), Cand.SU->getHeight(), TryCand, Cand,
GenericSchedulerBase::BotHeightReduce);
return TryCand.Reason != NoCand;
@@ -156,16 +199,14 @@ bool SystemZPreRASchedStrategy::tryCandidate(SchedCandidate &Cand,
void SystemZPreRASchedStrategy::initPolicy(MachineBasicBlock::iterator Begin,
MachineBasicBlock::iterator End,
unsigned NumRegionInstrs) {
- // Avoid setting up the register pressure tracker for small regions to save
- // compile time. Currently only used for computeCyclicCriticalPath() which
- // is used for single block loops.
- MachineBasicBlock *MBB = Begin->getParent();
- RegionPolicy.ShouldTrackPressure =
- MBB->isSuccessor(MBB) && NumRegionInstrs >= 8;
+ // Avoid setting up the register pressure tracker unless needed to save
+ // compile time.
+ RegionPolicy.ShouldTrackPressure = NumRegionInstrs > TopRegionSUs;
// These heuristics has so far seemed to work better without adding a
// top-down boundary.
RegionPolicy.OnlyBottomUp = true;
+
BotIdx = NumRegionInstrs - 1;
this->NumRegionInstrs = NumRegionInstrs;
}
@@ -173,16 +214,18 @@ void SystemZPreRASchedStrategy::initPolicy(MachineBasicBlock::iterator Begin,
void SystemZPreRASchedStrategy::initialize(ScheduleDAGMI *dag) {
GenericScheduler::initialize(dag);
+ RemLat = ~0U;
Cmp0SrcReg = SystemZ::NoRegister;
+ initializeLivenessReduction();
initializeLatencyReduction();
- LLVM_DEBUG(dbgs() << "Latency scheduling " << (HasDataSequences ? "" : "not ")
- << "enabled for data sequences.\n";);
}
void SystemZPreRASchedStrategy::schedNode(SUnit *SU, bool IsTopNode) {
GenericScheduler::schedNode(SU, IsTopNode);
+ RemLat = ~0U;
+
const SystemZInstrInfo *TII = static_cast<const SystemZInstrInfo *>(DAG->TII);
MachineInstr *MI = SU->getInstr();
if (TII->isCompareZero(*MI))
diff --git a/llvm/lib/Target/SystemZ/SystemZMachineScheduler.h b/llvm/lib/Target/SystemZ/SystemZMachineScheduler.h
index 4fdfd92d192c3..e266d59d5a0b7 100644
--- a/llvm/lib/Target/SystemZ/SystemZMachineScheduler.h
+++ b/llvm/lib/Target/SystemZ/SystemZMachineScheduler.h
@@ -8,9 +8,12 @@
// -------------------------- Pre RA scheduling ----------------------------- //
//
-// SystemZPreRASchedStrategy performs latency scheduling in certain types of
-// regions where this is beneficial, and also helps copy coalescing and
-// comparison elimination.
+// SystemZPreRASchedStrategy reduces register pressure by scheduling a (live)
+// definition low if it does not cause another register to become live (all
+// uses live). In certain regions it then reduces the scheduled latency but
+// only if the SU that is higher (by Height) than the scheduled latency is as
+// well lower (by Depth) than the remaining latency. It also helps copy
+// coalescing and comparison elimination.
//
// -------------------------- Post RA scheduling ---------------------------- //
//
@@ -34,6 +37,7 @@ namespace llvm {
/// A MachineSchedStrategy implementation for SystemZ pre RA scheduling.
class SystemZPreRASchedStrategy : public GenericScheduler {
+ void initializeLivenessReduction();
void initializeLatencyReduction();
Register Cmp0SrcReg;
@@ -41,9 +45,20 @@ class SystemZPreRASchedStrategy : public GenericScheduler {
// compare with 0. If CCDef is true MI must also have an implicit def of CC.
bool definesCmp0Src(const MachineInstr *MI, bool CCDef = true) const;
- // True if the region has many instructions in def-use sequences and would
- // likely benefit from latency reduction.
- bool HasDataSequences;
+ // The highest SUs that are not to be scheduled "low" to reduce liveness.
+ struct : SmallPtrSet<const SUnit *, 12> {
+ bool count(const SUnit *SU) const {
+ return empty() || SmallPtrSet::count(SU);
+ }
+ } HighSUs;
+
+ // Return true if the instruction defines a register while all use operands
+ // are already live.
+ bool closesLiveRange(const SUnit *SU, ScheduleDAGMILive *DAG) const;
+
+ // Only call computeRemLatency() once before each scheduled node.
+ mutable unsigned RemLat;
+ unsigned getRemLat(SchedBoundary *Zone) const;
protected:
bool tryCandidate(SchedCandidate &Cand, SchedCandidate &TryCand,
diff --git a/llvm/test/CodeGen/SystemZ/args-22.ll b/llvm/test/CodeGen/SystemZ/args-22.ll
index ba422b65fc299..0d20cd1ebc7c1 100644
--- a/llvm/test/CodeGen/SystemZ/args-22.ll
+++ b/llvm/test/CodeGen/SystemZ/args-22.ll
@@ -124,8 +124,8 @@ define void @arg1(%Ty1 %A) {
; VECTOR-NEXT: lgrl %r1, Dst@GOT
; VECTOR-NEXT: vrepib %v1, 8
; VECTOR-NEXT: vsteb %v0, 8(%r1), 15
-; VECTOR-NEXT: vsrlb %v0, %v0, %v1
-; VECTOR-NEXT: vsteg %v0, 0(%r1), 1
+; VECTOR-NEXT: vsrlb %v1, %v0, %v1
+; VECTOR-NEXT: vsteg %v1, 0(%r1), 1
; VECTOR-NEXT: br %r14
store %Ty1 %A, ptr @Dst
ret void
@@ -351,14 +351,14 @@ define void @arg3(%Ty3 %A) {
;
; VECTOR-LABEL: arg3:
; VECTOR: # %bb.0:
-; VECTOR-NEXT: vl %v0, 0(%r3), 3
+; VECTOR-NEXT: vl %v0, 0(%r2), 3
+; VECTOR-NEXT: vl %v1, 0(%r3), 3
; VECTOR-NEXT: lgrl %r1, Dst@GOT
-; VECTOR-NEXT: vl %v1, 0(%r2), 3
-; VECTOR-NEXT: vsteb %v1, 8(%r1), 15
-; VECTOR-NEXT: vst %v0, 16(%r1), 3
-; VECTOR-NEXT: vrepib %v0, 8
-; VECTOR-NEXT: vsrlb %v0, %v1, %v0
-; VECTOR-NEXT: vsteg %v0, 0(%r1), 1
+; VECTOR-NEXT: vsteb %v0, 8(%r1), 15
+; VECTOR-NEXT: vrepib %v2, 8
+; VECTOR-NEXT: vsrlb %v2, %v0, %v2
+; VECTOR-NEXT: vst %v1, 16(%r1), 3
+; VECTOR-NEXT: vsteg %v2, 0(%r1), 1
; VECTOR-NEXT: br %r14
store %Ty3 %A, ptr @Dst
ret void
@@ -402,11 +402,11 @@ define void @call3() {
; VECTOR-NEXT: vlrepg %v1, 0(%r1)
; VECTOR-NEXT: vrepib %v2, 8
; VECTOR-NEXT: vslb %v1, %v1, %v2
+; VECTOR-NEXT: vl %v2, 16(%r1), 3
; VECTOR-NEXT: vo %v0, %v0, %v1
-; VECTOR-NEXT: vl %v1, 16(%r1), 3
; VECTOR-NEXT: la %r2, 176(%r15)
; VECTOR-NEXT: la %r3, 160(%r15)
-; VECTOR-NEXT: vst %v1, 160(%r15), 3
+; VECTOR-NEXT: vst %v2, 160(%r15), 3
; VECTOR-NEXT: vst %v0, 176(%r15), 3
; VECTOR-NEXT: brasl %r14, Fnptr@PLT
; VECTOR-NEXT: lmg %r14, %r15, 304(%r15)
@@ -601,15 +601,15 @@ define %Ty4 @ret4() {
; VECTOR-NEXT: brasl %r14, Fnptr@PLT
; VECTOR-NEXT: lb %r0, 164(%r15)
; VECTOR-NEXT: lh %r1, 166(%r15)
-; VECTOR-NEXT: lb %r4, 200(%r15)
+; VECTOR-NEXT: lb %r2, 200(%r15)
; VECTOR-NEXT: lde %f0, 160(%r15)
-; VECTOR-NEXT: l %r2, 168(%r15)
-; VECTOR-NEXT: lg %r3, 176(%r15)
+; VECTOR-NEXT: l %r3, 168(%r15)
+; VECTOR-NEXT: lg %r4, 176(%r15)
; VECTOR-NEXT: vl %v1, 184(%r15), 3
-; VECTOR-NEXT: stc %r4, 40(%r13)
+; VECTOR-NEXT: stc %r2, 40(%r13)
; VECTOR-NEXT: vst %v1, 24(%r13), 3
-; VECTOR-NEXT: stg %r3, 16(%r13)
-; VECTOR-NEXT: st %r2, 8(%r13)
+; VECTOR-NEXT: stg %r4, 16(%r13)
+; VECTOR-NEXT: st %r3, 8(%r13)
; VECTOR-NEXT: sth %r1, 6(%r13)
; VECTOR-NEXT: stc %r0, 4(%r13)
; VECTOR-NEXT: ste %f0, 0(%r13)
@@ -810,10 +810,10 @@ define void @arg6(%Ty6 %A) {
; VECTOR-NEXT: vsteb %v1, 24(%r1), 15
; VECTOR-NEXT: vrepib %v2, 8
; VECTOR-NEXT: vsteb %v0, 8(%r1), 15
-; VECTOR-NEXT: vsrlb %v1, %v1, %v2
-; VECTOR-NEXT: vsrlb %v0, %v0, %v2
-; VECTOR-NEXT: vsteg %v1, 16(%r1), 1
-; VECTOR-NEXT: vsteg %v0, 0(%r1), 1
+; VECTOR-NEXT: vsrlb %v3, %v1, %v2
+; VECTOR-NEXT: vsrlb %v2, %v0, %v2
+; VECTOR-NEXT: vsteg %v3, 16(%r1), 1
+; VECTOR-NEXT: vsteg %v2, 0(%r1), 1
; VECTOR-NEXT: br %r14
store %Ty6 %A, ptr @Dst
ret void
@@ -854,17 +854,17 @@ define void @call6() {
; VECTOR-NEXT: aghi %r15, -192
; VECTOR-NEXT: .cfi_def_cfa_offset 352
; VECTOR-NEXT: lgrl %r1, Src@GOT
+; VECTOR-NEXT: vgbm %v0, 0
; VECTOR-NEXT: vgbm %v1, 0
; VECTOR-NEXT: vleb %v1, 8(%r1), 15
; VECTOR-NEXT: vlrepg %v2, 0(%r1)
-; VECTOR-NEXT: vrepib %v3, 8
-; VECTOR-NEXT: vslb %v2, %v2, %v3
-; VECTOR-NEXT: vgbm %v0, 0
-; VECTOR-NEXT: vo %v1, %v1, %v2
; VECTOR-NEXT: vleb %v0, 24(%r1), 15
-; VECTOR-NEXT: vlrepg %v2, 16(%r1)
-; VECTOR-NEXT: vslb %v2, %v2, %v3
-; VECTOR-NEXT: vo %v0, %v0, %v2
+; VECTOR-NEXT: vlrepg %v3, 16(%r1)
+; VECTOR-NEXT: vrepib %v4, 8
+; VECTOR-NEXT: vslb %v2, %v2, %v4
+; VECTOR-NEXT: vslb %v3, %v3, %v4
+; VECTOR-NEXT: vo %v1, %v1, %v2
+; VECTOR-NEXT: vo %v0, %v0, %v3
; VECTOR-NEXT: la %r2, 176(%r15)
; VECTOR-NEXT: la %r3, 160(%r15)
; VECTOR-NEXT: vst %v0, 160(%r15), 3
diff --git a/llvm/test/CodeGen/SystemZ/atomicrmw-ops-i128.ll b/llvm/test/CodeGen/SystemZ/atomicrmw-ops-i128.ll
index 9271dc73e2725..bdb05c1dd1776 100644
--- a/llvm/test/CodeGen/SystemZ/atomicrmw-ops-i128.ll
+++ b/llvm/test/CodeGen/SystemZ/atomicrmw-ops-i128.ll
@@ -106,11 +106,11 @@ define i128 @atomicrmw_nand(ptr %src, i128 %b) {
; CHECK-NEXT: vl %v1, 0(%r3), 4
; CHECK-NEXT: .LBB4_1: # %atomicrmw.start
; CHECK-NEXT: # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT: vnn %v2, %v1, %v0
; CHECK-NEXT: vlgvg %r1, %v1, 1
; CHECK-NEXT: vlgvg %r0, %v1, 0
-; CHECK-NEXT: vnn %v1, %v1, %v0
-; CHECK-NEXT: vlgvg %r5, %v1, 1
-; CHECK-NEXT: vlgvg %r4, %v1, 0
+; CHECK-NEXT: vlgvg %r5, %v2, 1
+; CHECK-NEXT: vlgvg %r4, %v2, 0
; CHECK-NEXT: cdsg %r0, %r4, 0(%r3)
; CHECK-NEXT: vlvgp %v1, %r0, %r1
; CHECK-NEXT: jl .LBB4_1
diff --git a/llvm/test/CodeGen/SystemZ/bswap-09.ll b/llvm/test/CodeGen/SystemZ/bswap-09.ll
index a2d8273c89695..e8468acaee431 100644
--- a/llvm/test/CodeGen/SystemZ/bswap-09.ll
+++ b/llvm/test/CodeGen/SystemZ/bswap-09.ll
@@ -9,14 +9,14 @@ declare i128 @llvm.bswap.i128(i128 %a)
define i128 @f1(i128 %a, i128 %b, i128 %c) {
; CHECK-LABEL: f1:
; CHECK: # %bb.0:
-; CHECK-NEXT: vl %v1, 0(%r4), 3
-; CHECK-NEXT: vl %v2, 0(%r3), 3
+; CHECK-NEXT: vl %v0, 0(%r4), 3
+; CHECK-NEXT: vl %v1, 0(%r3), 3
; CHECK-NEXT: larl %r1, .LCPI0_0
-; CHECK-NEXT: vaq %v1, %v2, %v1
; CHECK-NEXT: vl %v2, 0(%r1), 3
-; CHECK-NEXT: vl %v0, 0(%r5), 3
-; CHECK-NEXT: vperm %v1, %v1, %v1, %v2
+; CHECK-NEXT: vl %v3, 0(%r5), 3
; CHECK-NEXT: vaq %v0, %v1, %v0
+; CHECK-NEXT: vperm %v0, %v0, %v0, %v2
+; CHECK-NEXT: vaq %v0, %v0, %v3
; CHECK-NEXT: vst %v0, 0(%r2), 3
; CHECK-NEXT: br %r14
%in = add i128 %a, %b
@@ -32,9 +32,9 @@ define i128 @f2(i128 %a, i128 %b) {
; CHECK-NEXT: vl %v0, 0(%r4), 3
; CHECK-NEXT: vl %v1, 0(%r3), 3
; CHECK-NEXT: larl %r1, .LCPI1_0
+; CHECK-NEXT: vl %v2, 0(%r1), 3
; CHECK-NEXT: vaq %v0, %v1, %v0
-; CHECK-NEXT: vl %v1, 0(%r1), 3
-; CHECK-NEXT: vperm %v0, %v0, %v0, %v1
+; CHECK-NEXT: vperm %v0, %v0, %v0, %v2
; CHECK-NEXT: vst %v0, 0(%r2), 3
; CHECK-NEXT: br %r14
%in = add i128 %a, %b
@@ -47,11 +47,11 @@ define i128 @f3(i128 %a, i128 %b) {
; CHECK-LABEL: f3:
; CHECK: # %bb.0:
; CHECK-NEXT: larl %r1, .LCPI2_0
-; CHECK-NEXT: vl %v1, 0(%r3), 3
-; CHECK-NEXT: vl %v2, 0(%r1), 3
-; CHECK-NEXT: vl %v0, 0(%r4), 3
-; CHECK-NEXT: vperm %v1, %v1, %v1, %v2
-; CHECK-NEXT: vaq %v0, %v1, %v0
+; CHECK-NEXT: vl %v0, 0(%r3), 3
+; CHECK-NEXT: vl %v1, 0(%r1), 3
+; CHECK-NEXT: vl %v2, 0(%r4), 3
+; CHECK-NEXT: vperm %v0, %v0, %v0, %v1
+; CHECK-NEXT: vaq %v0, %v0, %v2
; CHECK-NEXT: vst %v0, 0(%r2), 3
; CHECK-NEXT: br %r14
%swapped = call i128 @llvm.bswap.i128(i128 %a)
diff --git a/llvm/test/CodeGen/SystemZ/bswap-10.ll b/llvm/test/CodeGen/SystemZ/bswap-10.ll
index 6de2970b80e2e..465c666808958 100644
--- a/llvm/test/CodeGen/SystemZ/bswap-10.ll
+++ b/llvm/test/CodeGen/SystemZ/bswap-10.ll
@@ -9,14 +9,14 @@ declare i128 @llvm.bswap.i128(i128 %a)
define i128 @f1(i128 %a, i128 %b, i128 %c) {
; CHECK-LABEL: f1:
; CHECK: # %bb.0:
-; CHECK-NEXT: vl %v1, 0(%r4), 3
-; CHECK-NEXT: vl %v2, 0(%r3), 3
+; CHECK-NEXT: vl %v0, 0(%r4), 3
+; CHECK-NEXT: vl %v1, 0(%r3), 3
; CHECK-NEXT: larl %r1, .LCPI0_0
-; CHECK-NEXT: vaq %v1, %v2, %v1
; CHECK-NEXT: vl %v2, 0(%r1), 3
-; CHECK-NEXT: vl %v0, 0(%r5), 3
-; CHECK-NEXT: vperm %v1, %v1, %v1, %v2
+; CHECK-NEXT: vl %v3, 0(%r5), 3
; CHECK-NEXT: vaq %v0, %v1, %v0
+; CHECK-NEXT: vperm %v0, %v0, %v0, %v2
+; CHECK-NEXT: vaq %v0, %v0, %v3
; CHECK-NEXT: vst %v0, 0(%r2), 3
; CHECK-NEXT: br %r14
%in = add i128 %a, %b
diff --git a/llvm/test/CodeGen/SystemZ/call-zos-vec.ll b/llvm/test/CodeGen/SystemZ/call-zos-vec.ll
index 20bf2687c957e..32d29cb8ebc08 100644
--- a/llvm/test/CodeGen/SystemZ/call-zos-vec.ll
+++ b/llvm/test/CodeGen/SystemZ/call-zos-vec.ll
@@ -9,15 +9,15 @@ entry:
}
; CHECK-LABEL: sum_vecs1
-; CHECK: vaf 1,24,25
-; CHECK: vaf 1,1,26
-; CHECK: vaf 1,1,27
-; CHECK: vaf 1,1,28
-; CHECK: vaf 1,1,29
-; CHECK...
[truncated]
|
This is simply a compile-time reduction without any effect on generated code, right? We should probably just commit that change to current mainline, it's really independent of this PR. I do have a question on the |
Yes - I removed it for now.
This heuristic has evolved around "decoder cycles", imagining a sweet spot for a "top region" where the OOO execution with branch misses etc could benefit from respecting latencies more than later on (in a big BB), when decoding is ahead anyway. Or, the other way around, it's ok to "pull down" a load/instruction with a latency close to its user to close a live range if it's below this threshold. I did benchmarking between 2 and 11 top cycles (1 cycle = 6 instructions) and found a value of 5 or 6 to give the better results. It wouldn't matter much if it was 30 or 40 instructions, probably, but I would worry to not build this set because there likely are regions with many SUs of the same height(s), meaning that this would become less precise if just a cut-off height value would be used. Note that the NodeNum is the decider here, which means that the input order is preserved in cases of equal height. I don't think the overhead is ever an issue, but I could try a simple cutoff and see if the performance remains. Maybe as a follow-up at some point? |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
uweigand
left a comment
There was a problem hiding this comment.
I agree this looks better, thanks! A few comment inline.
| int RegWeight = TRI->getRegClassWeight(RC).RegWeight; | ||
| bool VR16DefNoKill = VR16PChange == -RegWeight; | ||
| bool GRX32DefNoKill = GRX32PChange == -RegWeight; | ||
| return VR16DefNoKill || (!VR16PChange && GRX32DefNoKill); |
There was a problem hiding this comment.
What is the point of comparing this exactly against -RegWeight ? Wouldn't it be sufficient to check whether the change is negative? I.e. something like
return VR16PChange < 0 || (!VR16PChange && GRX32PChange < 0);
There was a problem hiding this comment.
The idea is to "do nothing" in the case of a reg-def together with a non-live use-operand present. Just checking for negative is also reasonable to me, but I have sofar avoided the case of a subreg-use in order to be slightly less aggressive so that this is only done when a live range is closed and no (subreg) uses would become live.
I tried this now with building SPEC, and saw 84 files changing. It seems this could involve either FP128 or (more likely) GR64 / GR32. I'll run it and see if it makes any difference in performance (probably not).
There was a problem hiding this comment.
So, overall this seems to be just a slight improvement both in regards to spilling and performance.
main (9b3f3b9, May 11) <> patch with -RegWeight:
Improvements:
0.916: f538.imagick_r
0.943: i500.perlbench_r
0.963: f519.lbm_r
0.984: i525.x264_r
0.989: i505.mcf_r
Regressions:
1.024: f511.povray_r // povray main time: 513.25s
patch per above <> patch without -RegWeight:
Improvements:
0.996: i523.xalancbmk_r
0.996: f508.namd_r
0.996: f519.lbm_r
Regressions:
1.004: f511.povray_r
So, the results are very similar, but a bit unfortunate that povray got worse. One povray file changed, and it had 20 less spill/restore instructions, over SPEC, 154 less (and also less regmoves). So it seems to be working as expected - slightly more active in reducing register liveness.
I have removed the -RegWeight from the patch as it seems to not help much at the moment.
| // liveness only when the decoder is ahead of execution anyway. | ||
| static cl::opt<unsigned> TopRegionSUs("top-region", cl::Hidden, cl::init(36)); | ||
| static cl::opt<bool> DisableLatency("disable-latency", cl::Hidden, | ||
| cl::init(false)); |
There was a problem hiding this comment.
I guess these will be removed before committing?
There was a problem hiding this comment.
Hmm, yeah - once we are ready and decided on a version I think I could try to derive some new tests instead of using these options.
|
|
||
| if (RegionPolicy.ShouldTrackPressure) { | ||
| float Ratio = DAG->SUnits.size() < 50 ? 0.25 : 0.5; | ||
| LivenessHeightCutOff = std::floor(Ratio * float(DAGHeight)); |
There was a problem hiding this comment.
I don't think we should be using floating-point computation here, and it's also completely unnecessary. The "floor" of a division by 2 or 4 can be done via integer arithmetic (or a simple shift).
There was a problem hiding this comment.
Ah, yeah, with these exact ratios this is not needed.
|
Patch updated per review. |
uweigand
left a comment
There was a problem hiding this comment.
Thanks, just a couple of minor nits.
| @@ -8,66 +8,27 @@ | |||
|
|
|||
| #include "SystemZMachineScheduler.h" | |||
| #include "llvm/CodeGen/MachineLoopInfo.h" | |||
| #include <cmath> | |||
There was a problem hiding this comment.
This shouldn't be needed any more.
|
|
||
| // Return true for a (vreg) def when register pressure is reduced. Prioritize | ||
| // FP/vector regs over GPRs. | ||
| if (isRegDef(SU->getInstr()->getOperand(0))) |
There was a problem hiding this comment.
Why do we need this if? Shouldn't the PressureDiff computation have considered this already?
There was a problem hiding this comment.
You're right - it seems completely NFC to remove this.
|
patch updated per review (NFC).
Is this version acceptable (without any further constraints on the latency reduction)? If so, a final step would be to rework the testing and get rid of the two options that they currently need. |
The code as such looks good to me now - of course, assuming it ends up OK performance-wise (and we decide on the choice of option we want). |
|
Experimental options removed with new tests that work without them. I also tried again if caching of computeRemLatency() would be meaningful, but it didn't seem to make any difference at all on either worst case or the average pass time (accross SPEC). |
uweigand
left a comment
There was a problem hiding this comment.
This looks to be an incremental improvement from a performance perspective, and it does also simplify the code, so we should just go for it. LGTM, thanks!
|
Hello. The two new tests added ( |
… mir tests (#205403) This is based off llvm/llvm-project#188823 and is needed because tests are failing in release (non-asserts) builds
… mir tests (#205403) This is based off llvm/llvm-project#188823 and is needed because tests are failing in release (non-asserts) builds
…88823) Add some handling of register pressure by scheduling an SU "low" if it closes a live range (under certain conditions). As this is checked before latency reduction, the "data-sequnces" check that was used to selectively enable latency reduction can now be removed. This gives good improvements on several benchmarks and is also a simplification of the SystemZPreRASchedStrategy.
…lvm#205403) This is based off llvm#188823 and is needed because tests are failing in release (non-asserts) builds
…(#205403) This is based off llvm/llvm-project#188823 and is needed because tests are failing in release (non-asserts) builds
Remove the "data-sequnces" check for latency reduction - this makes latency reduction happen in more regions.
Add some handling of liveness reduction by means of scheduling an SU that closes a live range "low".
The wrapping of computeRemLatency() in getRemLat(): With increased usage of the latency heuristic, this gives a slight improvement: Worst case is the same (also against main), but on average, the pass time goes from 1.61% vs 1.67% without it. Not sure if this could simply be dropped still (given the marginal improvement), and/or be suggested for GenericScheduler (common-code).
Testing: In order to use the test for liveness handling I had since before (misched-prera-loads.mir), two options were needed. TopRegionsSUs and DisableLatency were added just for this test, and I am not sure if that is ok or not:
A. Keep as is: It makes sense in a way to have these options and do the test this way: with a smaller top region (12 instead of 36 instructions), the tests become more manageable, and disabling the latency - that now happens more often - is also useful as it would likely interfere.
B. Remove these options and try to have some testing for liveness even if it means bigger regions (+36 instructions) and maybe not as readable and detailed (some new tests).
C. Just remove the options and skip these scheduling tests. They have been great during experiments, but they are probably covered implicitly by all the other non-related tests that have been updated. TopRegionSUS would then just be a constant.
Statistics:
Conclusion: Just like last time this was tried without the data-sequences heuristic these numbers deteriorate somewhat. However, performance measurements indicate that this does not matter in the end - the handling of latencies and "top-of-region" seems to be beneficial in this regard despite these numbers. As a follow-up it may be of interest to try to either improve on the cmp-elim results (did not work last time), or perhaps remove that handling if it doesn't help much.
(Before rebase, 3dfeeff was NFC with 19c0077.)