Page MenuHomePhabricator

[PowerPC] fix killed/dead flag after reg+reg to reg+imm transformation
ClosedPublic

Authored by shchenz on Feb 19 2019, 11:30 PM.

Details

Summary
---
name: testKilled
tracksRegLiveness: true
body: |
  bb.0.entry:
    liveins: $x3, $f1, $x5
    $x3 = ADDI8 $x5, 100
    STD killed $x5, $x5, 100
    STFSX killed $f1, $zero8, killed $x3
    BLR8 implicit $lr8, implicit $rm

llc -mtriple=powerpc64le--linux-gnu -stop-after ppc-pre-emit-peephole t.mir -o - -verify-machineinstrs

# After PowerPC Pre-Emit Peephole
# Machine code for function testKill: NoPHIs, TracksLiveness, NoVRegs

bb.0.entry:
  liveins: $f1, $x3, $x5
  STD killed $x5, $x5, 100
  STFS killed $f1, 100, $x5
  BLR8 implicit $lr8, implicit $rm

# End machine code for function testKill.

*** Bad machine code: Using an undefined physical register ***
- function:    testKill
- basic block: %bb.0 entry (0x100313d8218)
- instruction: STFS killed $f1, 100, $x5
- operand 2:   $x5
LLVM ERROR: Found 1 machine code errors.

PowerPC Peephole will convert ADDI8 + STFSX to STFS. But currently it does not consider about killed/dead flag, so after transformation in the above case $x5 is used after killed.

Diff Detail

Repository
rL LLVM

Event Timeline

shchenz created this revision.Feb 19 2019, 11:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 11:30 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
jsji requested changes to this revision.Feb 22 2019, 1:30 PM
jsji added inline comments.
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
1143 ↗(On Diff #187523)

This file was changed only by adding or removing whitespace. - Please avoid changing this in this patch.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2421 ↗(On Diff #187523)

We not only fixup Kill flag, but also set Dead flag, so maybe we should update the name?

2436 ↗(On Diff #187523)
(MI<->DefMI)

->

(MI -> DefMI]
2439 ↗(On Diff #187523)

Why It++; here? Can we add comments to explain?

2441 ↗(On Diff #187523)

Is it possible that debug instruction will mess up the logic here? Do we need to skip debug instructions?

2444 ↗(On Diff #187523)

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?

2459 ↗(On Diff #187523)

Is it possible that we have multiple same operands with killed flag set?

2487 ↗(On Diff #187523)

Why ForwardOperandReg instead of ForwardingOperandReg here?

3353 ↗(On Diff #187523)

Killed -> killed

3410 ↗(On Diff #187523)

Should we dump the instruction after flag fix?

3476 ↗(On Diff #187523)

Killed -> killed

llvm/lib/Target/PowerPC/PPCInstrInfo.h
414 ↗(On Diff #187523)

Can we add some doxygen comments here ? Especially relationship between MI and RegNo.

414 ↗(On Diff #187523)

Also, why this needs to be "Public" here? Is it intended for reuse somewhere else?

This revision now requires changes to proceed.Feb 22 2019, 1:30 PM
shchenz updated this revision to Diff 188100.Feb 24 2019, 9:29 PM
shchenz marked 13 inline comments as done.

address @jsji comments

shchenz marked 6 inline comments as done and 4 inline comments as done.Feb 24 2019, 9:33 PM
shchenz added inline comments.
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
1143 ↗(On Diff #187523)

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.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2439 ↗(On Diff #187523)

done.

2441 ↗(On Diff #187523)

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.

2444 ↗(On Diff #187523)

Yes, this is by design. Before we call this function, we should make sure RegNo liveness is killed in MI.

2459 ↗(On Diff #187523)

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$
3410 ↗(On Diff #187523)

Updated. Should dump after fix.

llvm/lib/Target/PowerPC/PPCInstrInfo.h
414 ↗(On Diff #187523)

This is by design. FixupKilledDeadFlags is designed to be a general interface for all PostRA transformations which will violate killed/flags semantics. So it is maybe called like TII->FixupKilledDeadFlags().

shchenz marked 5 inline comments as done.Feb 24 2019, 9:34 PM

Thanks for your comments. @jsji

Ready for new round review.

jsji requested changes to this revision.Feb 25 2019, 7:37 AM

Thanks for quick update, some more comments.

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
1143 ↗(On Diff #187523)

Then please use another NFC patch to correct that, don't try to squeeze into this one. Thanks.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2439 ↗(On Diff #188100)

is handled at the beginning -> has been handled above?

2421 ↗(On Diff #187523)

I don't like fixUpKilledDeadFlags, KilledDead looks confusing... and why fixUp not fixup?

How about fixupIsDeadOrKill ?

2441 ↗(On Diff #187523)

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?

2444 ↗(On Diff #187523)

Can we assert on this assumption?

2459 ↗(On Diff #187523)

Is it possible that we have multiple same operands with killed flag set in the same MI?

2459 ↗(On Diff #187523)
// 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?

2459 ↗(On Diff #187523)

Also, please add this situation into testcase?

llvm/lib/Target/PowerPC/PPCInstrInfo.h
418 ↗(On Diff #188100)

StartMI -> \p StartMI
EndMI -> \p EndMI
RegNo -> \p RegNo

llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs-kill-flag.mir
4 ↗(On Diff #188100)

Can we please add comments about test point for all the cases here? It is not explicit to me without comparing them carefully.

26 ↗(On Diff #188100)

This is copied from convert-rr-to-ri-instr-add.mir ? If this is already covered there, maybe we can skip it here?

80 ↗(On Diff #188100)

This is copied from convert-rr-to-ri-instr-add.mir ? If this is already covered there, maybe we can skip it here?

This revision now requires changes to proceed.Feb 25 2019, 7:37 AM
shchenz updated this revision to Diff 188366.Feb 26 2019, 6:49 AM
shchenz marked 10 inline comments as done.

address comments

shchenz updated this revision to Diff 188495.Feb 26 2019, 6:47 PM

address @jsji comments.

jsji accepted this revision.Mar 4 2019, 8:09 AM

LGTM, except some minor update. Thanks for fixing!

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2466 ↗(On Diff #188495)

This is almost identical to line 2442 , and it is for a special case handling, can we make it a lamda and just add the call here?

This revision is now accepted and ready to land.Mar 4 2019, 8:09 AM
shchenz updated this revision to Diff 189268.Mar 4 2019, 7:35 PM

address comments

This revision was automatically updated to reflect the committed changes.