[PATCH] D58428: [PowerPC] fix killed/dead flag after reg+reg to reg+imm transformation
Jinsong Ji via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 25 07:37:25 PST 2019
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
More information about the llvm-commits
mailing list