[PATCH] D56772: [MIR] Add simple PRE pass to MachineCSE

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/MachineCSE.cpp:814
+        unsigned NewReg = MRI->cloneVirtualRegister(VReg);
+        if (!isProfitableToCSE(NewReg, VReg, CMBB, MI))
+          continue;
----------------
anton-afanasyev wrote:
> LuoYuanke wrote:
> > Do we need to enhance the algorithm to consider more about register pressure on the profit calculation? I'm afraid there is performance drop when the register pressure is heavy.
> Hi @LuoYuanke, yes, this could be the case. Actually this commit doesn't change profit calculation, `ProcessBlockPRE()` uses the same `isProfitableToCommit()` function as `ProcessBlockCSE()` uses. Do you have concrete test cases where register pressure increases?
CSE only eliminate MI, but current PRE insert MI to the common dominated block, so the instruction is hoisted to dominated block. It increase register pressure more than CSE.
I have some cases that got performance regression with this patch due to the register pressure, but I am not able to extract a small case from it.
I notice in LICM,  MachineLICMBase::IsProfitableToHoist() is more considerate for the register pressure. I wonder if PRE can apply the same algorithm. Do you have any idea for a better register pressure solution?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56772