[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:
> > 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?
> Hi @LuoYuanke, actually PRE doesn't hoist instruction, but _duplicates_ it, real hoisting is made by CSE when it _eliminates_ instruction. I do believe that CSE should consider register pressure while eliminating instruction.
> 
> Have you tried `MachineLICMBase::isProfitableToHoist()` with your test case? We can adopt and insert it to CSE.
Hi @anton-afanasyev, sorry for reply late. I'm trying to figure out this issue, but it seems complex for me. Can you run SPEC cpu2017/500.perlbench_r? There is some significant performance drop on X86 with the patch. If you have resource to run the benchmark, could you help to look into the issue?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56772