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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Overall I think that this patch makes sense to me. A few minor things:

1. I think that there are some changes that are nice improvements but aren't really part of the kill flags fix. This is already a fairly large patch and removing these little fixes might make the patch clearer and easier to read. They can be made before or after with a (or several) NFC change(s).
2. I know this is an older patch and I apologize for taking so long to look at it. When I tried to rebase this patch to the Top of `main` I realized that I had to add a couple more `addRegToUpdate` in order to get it working. This isn't an issue with the patch it's just that new changes to `PPCMIPeephole` will require additions or `addRegToUpdate`.
3. Is there any way to provide an error or warning for the future when more peephole optimizations are added and for whatever reason the developer forgets to call `addRegToUpdate`? There may not be a way to do this and that's fine. I'm more curious.



================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:173
+static void addRegToUpdateWithLine(SmallSet<Register, 16> &RegsToUpdate,
+                                   Register Reg, int Line) {
+  if (!Register::isVirtualRegister(Reg))
----------------
If you made this function a private member of `PPCMIPeephole` then you wouldn't have to pass in `RegsToUpdate` since that `SmallSet` is already a private member of `PPCMIPeephole`. It would just save constantly having to repeat the first parameter that is always the same.

Of course if you did change this you would also have to make `convertUnprimedAccPHIs` a private member which I'm not sure is worth it... But anyway, maybe this is work for the future.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1144
       case PPC::RLDICR: {
-        Simplified |= emitRLDICWhenLoweringJumpTables(MI) ||
+        Simplified |= emitRLDICWhenLoweringJumpTables(MI, ToErase) ||
                       combineSEXTAndSHL(MI, ToErase);
----------------
nit:
Adding `ToErase` here is a good idea but maybe it should be a separate cleanup patch.
I don't think it has anything to do with the kill flag computation.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1235
+  for (Register Reg : RegsToUpdate) {
+    if (!MRI->reg_empty(Reg) && Register::isVirtualRegister(Reg))
+      LV->recomputeForSingleDefVirtReg(Reg);
----------------
Don't we guarantee that `RegsToUpdate` has only virtual registers?
```
static void addRegToUpdateWithLine(SmallSet<Register, 16> &RegsToUpdate,
                                   Register Reg, int Line) {
  if (!Register::isVirtualRegister(Reg))
    return;
...
```
Maybe I'm missing something but I feel like we don't need to check `&& Register::isVirtualRegister(Reg)`. 

What you may be able to do is add an assert to make sure that what I'm saying is actually true all the time.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1736
       LLVM_DEBUG(CMPI2->dump());
-    }
+    } else
 
----------------
lei wrote:
> Is this change on purpose to fix a previous bug?  Seems unrelated.
I'm also curious about this. I can't figure out why this `else` was added.
Should `Simplified` only be true if `!IsPartiallyRedudant`?


================
Comment at: llvm/test/CodeGen/PowerPC/peephole-phi-acc.mir:780
+    ; CHECK-NOT: accrc = PHI
+    ; CHECK: %2:uaccrc = PHI
     ; CHECK-NEXT: %39:vsrc
----------------
This looks like a fix for this test case that isn't related to the kill flags fix. Maybe a separate NFC patch? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133103