add a new goal MustReduceRegisterPressure for machine combiner pass. This is an NFC patch.
PowerPC will use this new goal to do some register pressure related optimization. https://reviews.llvm.org/D92071
Differential D92068
[MachineCombiner] [NFC]Add MustReduceRegisterPressure goal shchenz on Nov 24 2020, 7:40 PM. Authored by
Details
add a new goal MustReduceRegisterPressure for machine combiner pass. This is an NFC patch. PowerPC will use this new goal to do some register pressure related optimization. https://reviews.llvm.org/D92071
Diff Detail
Event TimelineComment Actions 1: fix lint warning messages and gentle ping... Comment Actions I'm not too familiar with MIR and I have not worked on MachineCombiner in a long time, so someone else should probably have a look too.
Comment Actions @spatel Very much appreciate your comments. It is a good start for this patch to move on.
For the dead flag in case test/DebugInfo/X86/machinecse-wrongdebug-hoist.ll, I think now it is more accurate. It should be an improvement. static bool isOperandKill(const MachineOperand &MO, MachineRegisterInfo *MRI) { return MO.isKill() || MRI->hasOneNonDBGUse(MO.getReg()); } yeah, sure, if this is a must fix degradation, I will fix this in follow up patches.
Compile-time diff: https://llvm-compile-time-tracker.com/compare.php?from=1bb79875e4b8f9018142a5155ca3f7df37778419&to=16f8136e47fdf13a05a27bbd0829d670f3baacb1&stat=instructions Thanks again.
Comment Actions I don't know the answer to that question - does it potentially harm other passes if the kill flag was dropped, or can we assume it does not matter? Adding more potential reviewers.
I assume it's an x86 machine, but @nikic can confirm.
We have tried to avoid changes that can cost that much compile-time without any benefit. If every target will have that loss even if there is no chance of run-time improvement, we should look for ways to hide it. Again, I'm not very familiar with how things work here - is it possible to make the extra analysis conditional on some flags internal to the machine combiner pass? Comment Actions I agree, this shouldn't incur compile-time cost for targets on which this won't even be run. Comment Actions Thanks for your comments @spatel @lebedev.ri Do you have any idea to run the LiveIntervals pass only on the target that MustReduceRegisterPressure is enabled? Comment Actions 1: don't require LiveIntervals analysis Comment Actions Hi @spatel @lebedev.ri , I changed the patch not requiring LiveIntervals pass. Now this patch is NFC. New compiling time diff is: https://llvm-compile-time-tracker.com/compare.php?from=5bb28fa0f51e94522644fe5633877b441b9ad8d3&to=73b8523c4c149866690257f58bc678be20a4991b&stat=instructions Could you help to have another look? Thanks. Comment Actions If this is NFC now, no objections from me - I just noted minor errors.
Comment Actions Thanks for your careful review. Updated accordingly. Thanks again for making this patch go so far. |
typo: "if target supports reassociation of instructions"