[PATCH] D92068: [MachineCombiner] Add MustReduceRegisterPressure goal

Fri Dec 4 00:49:15 PST 2020

shchenz added a comment.

@spatel  Very much appreciate your comments. It is a good start for this patch to move on.

> 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.

> 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 ^^.
GEO improves 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.

Thanks again.

================
Comment at: llvm/test/DebugInfo/X86/machinecse-wrongdebug-hoist.ll:3

-; CHECK: %5:gr32 = SUB32ri8 %0:gr32(tied-def 0), 1, implicit-def $eflags, debug-location !24; a.c:3:13
+; CHECK: dead %5:gr32 = SUB32ri8 %0:gr32(tied-def 0), 1, implicit-def $eflags, debug-location !24; a.c:3:13
 ; CHECK-NEXT: %10:gr32 = MOVSX32rr8 %4:gr8
----------------
This should be a positive change. register `%5` has no user below.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92068