We have a set of general purpose registers defined for RISC-V (32),
after which we can model the register pressure, inspired by the way it's
modeled in PPC. The motivation for this change is to get MachineCombiner
to not increase register pressure when re-ordering instructions.
Unfortunately, it's very hard to tell if a particular MICombiner
transform will increase register pressure without actually doing the
transform and re-computing register pressure. The workaround that LLVM
has for this is to predicate the transform on
shouldReduceRegisterPressure(), which should ideally tell us when we're
running out of registers. Hence, implement
shouldReduceRegisterPressure() for RISC-V, using the number of general
purpose registers as the limit.
Details
- Reviewers
craig.topper asb reames kito-cheng
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Include test, from a benchmark run reduced by hand.
I know the test isn't pretty, but it's the best I could do within reasonable time.
Could you create a phabricator patch only contain the pre-commit test and let this commit depend on it?
It could be easier to observe the difference in the test case.
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
---|---|---|
1381 | Captured all variable but shadowed MBB from argument of shouldReduceRegisterPressure, this could be a separated static function rather than lambda function I think? | |
1573–1595 | It seems like just a NFC refactor? if so this should split into a separated patch. |
Make getMBBPressure() a static function.
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
---|---|---|
1573–1595 | It's not an NFC refactor. Notice if (DoRegPressureReduce) return false; |
Does here some benchmark result on this change? I guess small benchmarks like dhrystone or coremark might not changed at all, so maybe spec cpu?
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
---|---|---|
1412 | Mention it's GPR class in debug info | |
1596 | I would like to have few more comment to mention we are only tracking reg pressure on GPR here. |
Unfortunately, we currently only have an embedded chip, and can't run SPEC. Dhrystone and Coremark are unchanged. There are, however, minor improvements observed in the Embench suite. Here's a full run-down of the results:
Program upstream upstream+patch diff test-suite :: External/Embench/ud.test 1914436 1915914 0.1% test-suite...xternal/CoreMark/coremark.test 1197810 1197810 0.0% test-suite...ternal/Embench/aha-mont64.test 2693624 2693624 0.0% test-suite...External/Embench/wikisort.test 906581 906581 0.0% test-suite...xternal/Embench/statemate.test 123791 123791 0.0% test-suite :: External/Embench/st.test 3418254 3418254 0.0% test-suite... :: External/Embench/slre.test 2000638 2000638 0.0% test-suite...al/Embench/sglib-combined.test 2476950 2476950 0.0% test-suite... External/Embench/qrduino.test 2354397 2354397 0.0% test-suite...ternal/Embench/primecount.test 3926956 3926956 0.0% test-suite...External/Embench/picojpeg.test 2200313 2200313 0.0% test-suite...External/Embench/nsichneu.test 2309555 2309555 0.0% test-suite...ternal/Embench/nettle-aes.test 2238164 2238164 0.0% test-suite...:: External/Embench/nbody.test 3044262 3044262 0.0% test-suite...: External/Embench/minver.test 516237 516237 0.0% test-suite...: External/Embench/md5sum.test 1155977 1155977 0.0% test-suite...ernal/Embench/matmult-int.test 1270437 1270437 0.0% test-suite...xternal/Embench/huffbench.test 1794048 1794048 0.0% test-suite :: External/Embench/edn.test 2016194 2016194 0.0% test-suite...:: External/Embench/cubic.test 5258870 5258870 0.0% test-suite...:: External/Embench/crc32.test 2962144 2962144 0.0% test-suite...marks/Dhrystone/dhrystone.test 226789 226789 0.0% test-suite... External/Embench/tarfind.test 2141216260 2140872390 -0.0% test-suite...nal/Embench/nettle-sha256.test 1757618 1754320 -0.2% Geomean difference -0.0%
I reviewed and saw this is similar to other target, but I hesitate to give LGTM to this patch since I saw the test case in early version of this patch shows stack usage increases[1], which typically means the register pressure increase, so that means this patch is not always did what we expect to me.
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
---|---|---|
1407 | Should this be using RegisterClassInfo::getRegPressureSetLimit which will take into account Reserved registers? I see uses of that in RegPressureTracker |
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
---|---|---|
1401 | Does this get called for register classes other than GPR? If the GPR pressure is already over the limit, does that mean we return true even though the register class in question might be floating point? |
Due to some changes, this patch doesn't seem to be necessary any longer. Will revisit after llvm-17 if necessary.
Captured all variable but shadowed MBB from argument of shouldReduceRegisterPressure, this could be a separated static function rather than lambda function I think?