[PATCH] D133103: [PowerPC] Improve kill flag computation and add verification after MI peephole

[Kamau Bridgeman via Phabricator via llvm-commits]

kamaub added a comment.

Some comments from the llvm-on-power team:



================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:452
 
-          if (TII->convertToImmediateForm(MI)) {
+          SmallVector<Register, 2> UpdatedRegs;
+          if (TII->convertToImmediateForm(MI, UpdatedRegs)) {
----------------
>From the use of the variable, it seems more consistent for this to be a Smallset instead of a Smallvector, additionally, is 2 enough? We check the MI and the DefMI for Regs to update, a bigger set size may be more optimal.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:478
+    // on any of its definitions that are marked as needing an update since
+    // doing so later would cause problems as the VReg is deleted.
+    if (!RegsToUpdate.empty()) {
----------------
Please add a comment explaining why this is needed.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:479
+    // doing so later would cause problems as the VReg is deleted.
+    if (!RegsToUpdate.empty()) {
+      for (MachineOperand &MO : ToErase->operands()) {
----------------
```
if (RegsToUpdate.empty())
  return;
```
I think this is more appropriate to the coding guidelines.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:481
+      for (MachineOperand &MO : ToErase->operands()) {
+        if (MO.isReg() && MO.isDef() && RegsToUpdate.count(MO.getReg())) {
+          Register RegToUpdate = MO.getReg();
----------------
  # Please DeMorganize this into an early exit condition.
  # Please add a comment explaining why this needs to be a definition of a register.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133103