[PATCH] D92068: [MachineCombiner] Add MustReduceRegisterPressure goal

Sun Dec 6 10:43:29 PST 2020

spatel added reviewers: craig.topper, RKSimon, pengfei, nikic, lebedev.ri.
spatel added a comment.

In D92068#2433227 <https://reviews.llvm.org/D92068#2433227>, @shchenz wrote:

>> 1. Why are the `killed` and `dead` flags changing in the tests?
>
> 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.
> For the `kill` flags in other cases, it caused by `Liveintervals` pass cleaning the killed flag for uses. See https://github.com/llvm/llvm-project/blob/5b9fc44d8128717ef2f219b061c018abb85c717f/llvm/lib/CodeGen/LiveIntervalCalc.cpp#L157-L160.
> I am guessing this code can be improved because now `Liveintervals` is used in many places except before register allocation, such as `MachineScheduler`, `Machinepipeliner`, and some other target-specific passes.  Fortunately, before RA, we have data flow analysis, so it is easy for us to check if a register/operand is killed without depending on the kill flag. For example in `MachineLICM` pass, we check kill operand like:
>
>   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.

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.

>> 2. Is there compile-time increase from having the extra passes (Slot index numbering, Live Interval Analysis, Machine Trace Metrics) even for targets that do not enable the MustReduceRegisterPressure objective?
>
> Compile-time diff: https://llvm-compile-time-tracker.com/compare.php?from=1bb79875e4b8f9018142a5155ca3f7df37778419&to=16f8136e47fdf13a05a27bbd0829d670f3baacb1&stat=instructions
> I am not sure which target is for above testing, but I think it must be not PowerPC ^^.

I assume it's an x86 machine, but @nikic can confirm.

> GEO increases 0.53%, and biggest increase is 0.7%, do you think this is an acceptable increase after I add these new passes to machine combiner? 
> I think LiveIntervals pass can not only be used for this patch but it can also be used for later register pressure track for ILP related reassociations. I knew we used to plan a register estimation model in this pass to prevent too many reassociations from causing too big register pressures.

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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92068/new/

https://reviews.llvm.org/D92068