[PATCH] D58428: [PowerPC] fix killed/dead flag after reg+reg to reg+imm transformation

[Jinsong Ji via Phabricator via llvm-commits]

jsji requested changes to this revision.
jsji added a comment.
This revision now requires changes to proceed.

Thanks for quick update, some more comments.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1143
       if (MIB->readsRegister(Reg, TRI)) {
-        MachineOperand *MO = 
+        MachineOperand *MO =
             MIB->findRegisterDefOperand(Reg, false, false, TRI);
----------------
shchenz wrote:
> jsji wrote:
> > This file was changed only by adding or removing whitespace. - Please avoid changing this in this patch.
> The empty namespace is added by mistake in this patch's NFC patch https://reviews.llvm.org/rL354438.
> But I am ok to remove this change if your insist.
Then please use another NFC patch to correct that, don't try to squeeze into this one. Thanks.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2421
+// register RegNo.
+void PPCInstrInfo::fixUpKillFlags(MachineInstr &MI, MachineInstr &DefMI,
+                                  unsigned RegNo) const {
----------------
jsji wrote:
> We not only fixup `Kill` flag, but also set `Dead` flag, so maybe we should update the name?
I don't like `fixUpKilledDeadFlags`, `KilledDead` looks confusing... and why `fixUp` not `fixup`?

How about `fixupIsDeadOrKill` ?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2441
+  MachineOperand *MO = nullptr;
+  for (; It != E; ++It) {
+    // If killed is not set, set killed for its use or set dead for its def if
----------------
shchenz wrote:
> jsji wrote:
> > Is it possible that debug instruction will mess up the logic here? Do we need to skip debug instructions?
> debug instructions should not impact functionality of this function. findRegisterUseOperand/findRegisterDefOperand will return nullptr for debug instructions I think. 
> 
> But it impacts perf, I have updated.
Thanks for updating, 
but I am not sure that`findRegisterUseOperand/findRegisterDefOperand will return nullptr for debug instructions `, any spec say that debug instruction can NOT have Reg operands?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2444
+    // no use found.
+    if (!IsKillSet) {
+      if ((MO = It->findRegisterUseOperand(RegNo, false, &getRegisterInfo()))) {
----------------
shchenz wrote:
> jsji wrote:
> > If we don't set `IsKillSet` for `MI` in line 2433, is it by design to also allow updating `kill/dead` for `RegNo` here? eg: we call this function with a `RegNo` that is not used in `MI`, and last use before `MI` is not killed too?
> Yes, this is by design. Before we call this function, we should make sure RegNo liveness is killed in MI.
Can we assert on this assumption?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2459
+      // If killed is already set, clear old killed flag.
+      if ((MO = It->findRegisterUseOperand(RegNo, true, &getRegisterInfo()))) {
+        MO->setIsKill(false);
----------------
shchenz wrote:
> jsji wrote:
> > Is it possible that we have multiple same operands with killed flag set?
> Yes, we maybe meet this issue. We should continue to clear not-last-use instruction's killed flag for RegNo.
> 
>   // Fixup kill/dead flag after transformation.$
>   // Pattern 1:$
>   // x = op1 KilledFwdFeederReg, imm$
>   // n1 = opn1 KilledFwdFeederReg(killed), regn1$
>   // n2 = opn2 KilledFwdFeederReg(killed), regn2$
>   // y = op2 0, x$
Is it possible that we have multiple same operands with killed flag set **in the same MI**?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2459
+      // If killed is already set, clear old killed flag.
+      if ((MO = It->findRegisterUseOperand(RegNo, true, &getRegisterInfo()))) {
+        MO->setIsKill(false);
----------------
jsji wrote:
> shchenz wrote:
> > jsji wrote:
> > > Is it possible that we have multiple same operands with killed flag set?
> > Yes, we maybe meet this issue. We should continue to clear not-last-use instruction's killed flag for RegNo.
> > 
> >   // Fixup kill/dead flag after transformation.$
> >   // Pattern 1:$
> >   // x = op1 KilledFwdFeederReg, imm$
> >   // n1 = opn1 KilledFwdFeederReg(killed), regn1$
> >   // n2 = opn2 KilledFwdFeederReg(killed), regn2$
> >   // y = op2 0, x$
> Is it possible that we have multiple same operands with killed flag set **in the same MI**?
```
// Fixup kill/dead flag after transformation.$
// Pattern 1:$
// x = op1 KilledFwdFeederReg, imm$
// n1 = opn1 KilledFwdFeederReg(killed), regn1$
// n2 = opn2 KilledFwdFeederReg(killed), regn2$
// y = op2 0, x$
```

Hmm, should we set `killed` for `n1` MI here？



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2459
+      // If killed is already set, clear old killed flag.
+      if ((MO = It->findRegisterUseOperand(RegNo, true, &getRegisterInfo()))) {
+        MO->setIsKill(false);
----------------
jsji wrote:
> jsji wrote:
> > shchenz wrote:
> > > jsji wrote:
> > > > Is it possible that we have multiple same operands with killed flag set?
> > > Yes, we maybe meet this issue. We should continue to clear not-last-use instruction's killed flag for RegNo.
> > > 
> > >   // Fixup kill/dead flag after transformation.$
> > >   // Pattern 1:$
> > >   // x = op1 KilledFwdFeederReg, imm$
> > >   // n1 = opn1 KilledFwdFeederReg(killed), regn1$
> > >   // n2 = opn2 KilledFwdFeederReg(killed), regn2$
> > >   // y = op2 0, x$
> > Is it possible that we have multiple same operands with killed flag set **in the same MI**?
> ```
> // Fixup kill/dead flag after transformation.$
> // Pattern 1:$
> // x = op1 KilledFwdFeederReg, imm$
> // n1 = opn1 KilledFwdFeederReg(killed), regn1$
> // n2 = opn2 KilledFwdFeederReg(killed), regn2$
> // y = op2 0, x$
> ```
> 
> Hmm, should we set `killed` for `n1` MI here？
> 
Also, please add this situation into testcase?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2439
+  MachineBasicBlock::reverse_iterator E = EndMI.getParent()->rend();
+  // EndMI is handled at the beginning, skip it here.
+  It++;
----------------
`is handled at the beginning` -> `has been handled above`?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:418
+  /// semantics, this function is called to fix up. Before calling this
+  /// function, make sure RegNo liveness is killed in instruction EndMI.
+  void fixUpKilledDeadFlags(MachineInstr &StartMI, MachineInstr &EndMI,
----------------
StartMI -> \p StartMI
EndMI -> \p EndMI
RegNo -> \p RegNo


================
Comment at: llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs-kill-flag.mir:4
+---
+name: testKillPassUpLI1
+# CHECK: name: testKillPassUpLI1
----------------
Can we please add comments about test point for all the cases here? It is not explicit to me without comparing them carefully.


================
Comment at: llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs-kill-flag.mir:26
+    STFSX killed $f1, $x3, $x5
+    ; CHECK: STFS killed $f1, 100, $x5
+    STD killed $x3, killed $x5, 100
----------------
This is copied from `convert-rr-to-ri-instr-add.mir` ?  If this is already covered there, maybe we can skip it here?


================
Comment at: llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs-kill-flag.mir:80
+# CHECK: name: testKillPassUpADD2
+tracksRegLiveness: true
+body: |
----------------
This is copied from `convert-rr-to-ri-instr-add.mir` ?  If this is already covered there, maybe we can skip it here?


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

https://reviews.llvm.org/D58428