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

Fri Apr 28 07:01:29 PDT 2023

nemanjai marked 29 inline comments as done.
nemanjai added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:468
+        if (MO.isReg() && MO.isDef() && RegsToUpdate.count(MO.getReg())) {
+          RegsToUpdate.erase(MO.getReg());
+          LV->recomputeForSingleDefVirtReg(MO.getReg());
----------------
amyk wrote:
> We're accessing the register both in the cases when we're removing it from `RegsToUpdate` and when we're re-running LiveVariables.
> It might make sense to pull out `MO.isReg()` into a separate variable.
The call to `MO.getReg()` in the `if` statement is safe because of short circuiting of the call to `MO.isReg()`.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:173
+static void addRegToUpdateWithLine(SmallSet<Register, 16> &RegsToUpdate,
+                                   Register Reg, int Line) {
+  if (!Register::isVirtualRegister(Reg))
----------------
stefanp wrote:
> 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.
I think it is not unreasonable to make these two functions members. Their functionality is part of the pass so it makes sense for them to be members.
The alternative solution would be to make `RegsToUpdate` a `static` member and access it from the free functions. However, that has the very undesirable effect of making the pass not thread-safe which matters at least for LTO builds.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:789
               LLVM_DEBUG(dbgs() << "Removing redundant shift: ");
               LLVM_DEBUG(DefMI->dump());
               ToErase = DefMI;
----------------
amyk wrote:
> Do we need to `addRegToUpdate()` here?
I think so. Thanks for catching this.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:848
               RoundInstr->eraseFromParent();
+              addRegToUpdate(RegsToUpdate, ConvReg1);
             }
----------------
kamaub wrote:
> Should there be a call to `recomputeLVForDyingInstr()` as well since this `ConvReg1` is a reg of `RoundInstr` which is being `eraseFromParent()` on line 847 and cannot be marked `ToErase` inside the lambda?
No, because `ConvReg1` is not **defined** by `RoundInstr`.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:997
               .addImm(PPC::sub_32);
           ToErase = &MI;
           Simplified = true;
----------------
amyk wrote:
> Do we need to `addRegToUpdate()` here?
Actually no because we have actually added a new definition of the register that `MI` originally defined in the exact same place. The original `MI` will be deleted and the live range of the register it defined doesn't change.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1109
     // Reset TrapOpt to false at the end of the basic block.
-    if (EnableTrapOptimization)
-      TrapOpt = false;
----------------
lei wrote:
> Why was this removed?
I don't remember. I have put it back now.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1207
         // We will delete the MI if it will never trap.
         ToErase = &MI;
         Simplified = true;
----------------
kamaub wrote:
> Do we need to track the registers in this instruction
This is currently broken, so we can't really perform this optimization safely.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1235
+  for (Register Reg : RegsToUpdate) {
+    if (!MRI->reg_empty(Reg) && Register::isVirtualRegister(Reg))
+      LV->recomputeForSingleDefVirtReg(Reg);
----------------
stefanp wrote:
> 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.
Oh, good point. The check is redundant.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1722
     else {
       // We finally eliminate compare instruction in MBB2.
       BI2->getOperand(1).setReg(BI1->getOperand(1).getReg());
----------------
amyk wrote:
> Why do we not need to add `addRegToUpdate()` here when we finally eliminate the compare instruction?
Because we've added all the register operands of `CMPI1` and `CMPI2` starting on line 1663.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1724
       BI2->getOperand(1).setReg(BI1->getOperand(1).getReg());
       CMPI2->eraseFromParent();
     }
----------------
kamaub wrote:
> We add this compare's operands to the RegsToUpdate unconditionally earlier, and now in this code path it is deleted meaning that we could end up attempting to recompute the LV for its VRegs and could hit errors.
> If this is the case I think being inside a function means `recomputeLVForDyingInstr()` would have to become a static function in order to catch this corner case =?
Actually, we don't need to because there is only one user of the register it defines and that user is `BI2` which just had that use replaced so the only definition from `CMPI2` is going to be a dead register. We don't try to recompute live variables for dead registers. I've added a comment to explain this.

================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1736
       LLVM_DEBUG(CMPI2->dump());
-    }
+    } else

----------------
stefanp wrote:
> 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`?
Hmm, I don't actually remember, but it definitely looks wrong. Removed it.

================
Comment at: llvm/test/CodeGen/PowerPC/convert-ri-addi-to-ri.mir:2
 # RUN: llc -mtriple=powerpc64le-unknown-linux-gnu -verify-machineinstrs \
-# RUN:   -run-pass ppc-mi-peepholes -ppc-convert-rr-to-ri %s -o - | FileCheck %s
+# RUN:   -run-pass ppc-mi-peepholes,livevars -ppc-convert-rr-to-ri %s -o - | \
+# RUN:   FileCheck %s
----------------
amyk wrote:
> Potentially silly question, but because we're adding `livevars` here explicitly, does this mean we are not running the live variables pass by default when we are adding live variables to PPCMIPeephole?
Very good catch. This was a remnant of the initial implementation that simply added the `livevars` to the pipeline. I have removed it now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133103