improvesCriticialPathLen() currently has an early-exit on
CombinerObjective::MustReduceDepth introduced in 766589ef (add
'MustReduceDepth' as an objective/cost-metric for the MachineCombiner),
by only considering transforms that would reduce the root depth.
However, the patch was introduced because MachineCombiner's use of
MachineTraceMetrics was incomplete, as noted in the Phabricator
discussion. This patch attempts to make better use of
MachineTraceMetrics in MachineCombiner, by also computing instruction
heights, and asserting that the critical path length = depth + slack +
height. We then unify the cost model in improvesCriticalPathLen(), since
there are only two essential differences between the MustReduceDepth
case and the other case: the MustReduceDepth case doesn't want to
consider slack, and wants to be more conservative with respect to the
cost comparison.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
This patch gives us improvements on standard benchmarks like Embench.
A couple of things to note:
- I need some help with the height calculation, as it is wrong in some places.
- I've only updated the RISC-V tests. Will update tests for other architectures, once the patch is closer to final form.
llvm/lib/CodeGen/MachineCombiner.cpp | ||
---|---|---|
265 | Should it go through all users as following? for (MachineInstr &UseInstr : MRI->use_instructions(MO.getReg())) { |
Comment Actions
Theoretically, NewRootHeight and OldRootHeight should be equal.
Does it make sense to remove the Heights calculations?
llvm/lib/CodeGen/MachineCombiner.cpp | ||
---|---|---|
269 | In MachineTraceMetrics::Ensemble::computeInstrHeights function, Loop header PHI heights are all 0. It seems it could be addressed by defining. unsigned MachineTraceMetrics::Trace::getPHIHeight(const MachineInstr &PHI) const{ // Loop header PHI heights are all 0. return TBI.Succ ? getInstrCycles(PHI).Height : 0; } |
Comment Actions
You're absolutely right; I'll try to explain the improvements I'm seeing, and re-think this patch.
Should it go through all users as following?
for (MachineInstr &UseInstr : MRI->use_instructions(MO.getReg())) {