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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

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

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



================
Comment at: lib/CodeGen/OptimizePHIs.cpp:123-125
+      if (MI->getOperand(i+1).getMBB() == SrcMI->getParent()) {
+        // If the copy instr is in the predecessor BB (almost always for PHI
+        // reg operands), try to propagate copy's operand.
----------------
I don't undestand why COPYs would be more likely in the predecessor blocks of the PHIs (this is before PHI lowering after all). And it also doesn't make any sense to me to have different behavior depending on where the COPY is, can't we just always look through the COPY and change `SrcReg`?


================
Comment at: lib/CodeGen/OptimizePHIs.cpp:140
       // Fail if there is more than one non-phi/non-move register.
-      if (SingleValReg != 0)
+      if (SingleValReg != 0 && SingleValReg != SrcReg)
         return false;
----------------
This is a nice change!


================
Comment at: test/CodeGen/X86/opt_phis2.mir:15-35
+  - { id: 0, class: vr256, preferred-register: '' }
+  - { id: 1, class: vr256, preferred-register: '' }
+  - { id: 2, class: vr256, preferred-register: '' }
+  - { id: 3, class: vr256, preferred-register: '' }
+  - { id: 4, class: vr256, preferred-register: '' }
+  - { id: 5, class: vr256, preferred-register: '' }
+  - { id: 6, class: vr256, preferred-register: '' }
----------------
Things should also work without this block, as you already define all the register classes on the MIR operands themselfes.


================
Comment at: test/CodeGen/X86/opt_phis2.mir:36-39
+liveins:
+  - { reg: '$edi', virtual-reg: '%7' }
+  - { reg: '$ymm0', virtual-reg: '%8' }
+  - { reg: '$rsi', virtual-reg: '%9' }
----------------
I suspect you can simply drop this block as well for this test


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54839