[PATCH] D91391: [PowerPC] Fix for excessive ACC copies due to PHI nodes

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

My comments are all cosmetic, feel free to address them on the commit. LGTM otherwise.



================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:318
+// We keep a map to associate each changed PHI node to its non-changed form. The
+// function returns false if the optimization cannot be applied on these nodes.
+static void convertUnprimedAccPHIs(const PPCInstrInfo *TII,
----------------
This comment appears to be out of date. It talks about returning `false` but the return type is `void`. Please revise the comment.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:363
+    Register AccReg = Dst;
+    if (PHI != PHIs[0])
+      AccReg = MRI->createVirtualRegister(&PPC::ACCRCRegClass);
----------------
Please add a comment here describing why we are creating a new primed accumulator virtual register as it is not trivially obvious.
Perhaps something like
```
If the PHI node we are changing is the root node, the register it defines will
be the destination register of the original copy (of the PHI def). For all other
PHI's in the list, we need to create another primed accumulator virtual
register as the PHI will no longer define the unprimed accumulator.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:439
+        // If the input to the copy is a PHI that is fed only by (i) copies in
+        // the other directions (ii) implicitly defined unprimed accumulators or
+        // (iii) other PHI nodes satisfying (i) (ii) and (iii), we can change
----------------
s/directions/direction


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:440
+        // the other directions (ii) implicitly defined unprimed accumulators or
+        // (iii) other PHI nodes satisfying (i) (ii) and (iii), we can change
+        // the PHI to a PHI on primed accumulators (as long as we also change
----------------
s/satisfying (i) (ii) and (iii)/satisfying (i) and (ii)
no need for a recursive description :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91391