[PATCH] D54839: [CodeGen] Enhance machine PHIs optimization

Sun Dec 9 03:16:39 PST 2018

anton-afanasyev added a comment.

In D54839#1321539 <https://reviews.llvm.org/D54839#1321539>, @MatzeB wrote:

> I'm not too familiar with the history of `OptimizePHIs` but some things feel odd to me here:
>
> - The comments in the pass make it seem like it is designed to catch dataflow around loops, yet the motivation for your changes appears to be simpler phis without loops involved.

This pass is more about PHIs cycles rather than about dataflow loops (though they are related). I've just expanded this pass to work with more complex PHIs configuration. I wanted firstly to rename function `IsSingleValuePHICycle()` to something like `IsSingleValuePHICycleOrChain()` but then realized that before it was not only about cycle as well (PHIs configuration could contain several cycles). Actual name of function (before or after my change) should be `IsSingleValuePHIGraph()` or `IsSingleValuePHIConfiguration()`. The common sense remained the same -- we have several (possibly single) PHIs connected by cycle/cycles/chain with only single external register. You are right in the sense that with patch this pass can eliminate single PHI node without cycle to other PHI nodes, but one can still regard it as "PHI loop" to itself through COPYied register. And you are right that it was my motivation initially (for further MachineCSE/GVN simplification), but this case was expanded to more common fortunately. Also it triggered several enhancements in regression tests having phi cycles.
What I really forgot is to add some comments to this function description to make it more clear. Will it be enough?

> - I keep wondering why we bother with COPYs here at all. If the COPYs are really trivial COPYs between the same register classes then we really should just remove them when we are still in MachineSSA. Do you know how they are created? Maybe you can simply stop their creation instead?
> - Skipping COPYs like this is also a bit inconsequential and will miss several cases: You could have more than 1 COPY in a row or switch between register classes. The PeepholeOptimizer pass already handles these more complex cases, so maybe we rather should look for running OptimizePHIs after the Peephole Optimizer so those COPYs are optimized? (Not sure though if there are other negative effects of running OptimizePHIs later or PeepholeOpt earlier).

These COPYs are created while MIR instruction selection, for instance, llvm instr `%2 = bitcast <4 x i64> %1 to <8 x i32>` translates to `%2 = COPY %1`. Not sure this COPYs could be eliminated during X86 DAG->DAG itself. 
I'm not familiar with PeepholeOptimizer, are you sure it is aimed to handle such COPYs? I've tried `llc -run-pass=peephole-opt opt_phis2.mir`, haven't seen any COPYs propagation on my test sample `opt_phis2.mir`. Nevertheless, eliminating COPYs is not enough to optimize phis at the samples like this one. Without patch this pass even cannot optimize `%a = PHI %b, %bb1, %b, %bb2` to `%a = COPY %b`. Actually COPYs are auxilarly thing in this pass. I've explored MachineCSE pass before this patch coding -- it uses auxiliary copy propagation as well to help main optimization (see PerformTrivialCopyPropagation()). Actually OptimizePHI pass itself (without my patch) already uses copy propagation -- but only to get next possible PHI node, not to get possible same register. One can do copy propagation (in MachineSSA) in any preceding pass (which one?), or just add several lines in passes where it is needed (MachineCSE, OptimizePHIs, ...). After breaking SSA form such redundant COPIes will be eliminated anyway.
Yes, you're right, this pass checks only one COPY in a row but that is true before my patch as well, I've just added the case when several COPYs leads to the same register. Though I believe more than one COPY in a row is rare case.

> With all that said, I don't expect the patch to break anything, so no principal objections to committing it...

Thank you for deep review! I'm to remove redundant if-condition of COPY locations which you have noticed.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54839