[PATCH] D64220: [PowerPC] Remove redundant load immediate instructions

[Stefan Pintilie via Phabricator via llvm-commits]

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

I only have a number of very minor comments. Overall I think this looks good.
I think you can fix the comments when you commit.

LGTM.



================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:61
+    // This function removes any redundant load immediates. It has two level
+    // loops - The outer loop finds the load immediates BBI that could be use to
+    // replace following redundancy. The inner loop scans instructions that
----------------
nit:
use -> used


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:67
+    // DeadOrKillToUnset is a pointer to the previous operand has kill/dead
+    // flag. It tracks from the def register of BBI, use registers of AfterBBIs
+    // and def registers of AfterBBIs.
----------------
nit: 
Wording.
`DeadOrKillToUnset is a pointer to the previous operand has kill/dead flag.`
I think this next one sounds a little better:
`DeadOrKillToUnset is a pointer to the previous operand that had the kill/dead flag set.`

Similarly:
`It tracks from the def register of BBI, use registers of AfterBBIs and def registers of AfterBBIs.`
Sounds better:
`It keeps track of the def register of BBI, the use registers of AfterBBIs and the def registers of AfterBBIs.`

The above are just suggestions. If you think another wording sounds better you can use that.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:76
+      for (auto BBI = MBB.instr_begin(), BBE = MBB.instr_end(); BBI != BBE;
+           ++BBI) {
+        // Skip load immediate that is marked to be erased later because it
----------------
I think it would be easier to read this loop if you just used `MBB.instrs()` and went through the range that way. 
The only place you actually need BBE is as the loop bounds for the inner loop  and there you can use `MBB.instr_end()` directly.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:86
+          continue;
+        // Skip load immediate, which operand is a relocation (e.g., $r3 = LI
+        // target-flags(ppc-lo) %const.0).
----------------
nit:
`which operand is a relocation` to `where the operand is a relocation`


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:106
+        // redundant load immediate.
+        for (auto AfterBBI = std::next(BBI); AfterBBI != BBE; ++AfterBBI) {
+          // Track the operand that kill Reg. We would unset the kill flag of
----------------
So here you can probably just use `AfterBBI != MBB.instr_end()`. I know that technically saving the value once and using it in the loop is slightly more efficient in terms of performance but I tend to prefer code that is easier to read when the performance difference is really small like this. However, having said that, others might prefer the other way around so if others (you or other reviewers) prefer the other way around we can do that too.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:157
+        MI->eraseFromParent();
+        NumRemovedInPreEmit++;
+      }
----------------
You can use `NumRemovedInPreEmit += InstrsToErase.size()` outside of the loop.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:159
+      }
+      return InstrsToErase.size() > 0;
+    }
----------------
nit:
How about this?
`return !InstrsToErase.empty()`


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

https://reviews.llvm.org/D64220