This is an archive of the discontinued LLVM Phabricator instance.

[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

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
2429

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

2444
(MI<->DefMI)

->

(MI -> DefMI]
2447

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

2449

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

2452

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?

2467

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

2522

Why ForwardOperandReg instead of ForwardingOperandReg here?

3390

Killed -> killed

3463

Should we dump the instruction after flag fix?

3511

Killed -> killed

llvm/lib/Target/PowerPC/PPCInstrInfo.h
414

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

414

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
2447

done.

2449

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.

2452

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

2467

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$
3463

Updated. Should dump after fix.

llvm/lib/Target/PowerPC/PPCInstrInfo.h
414

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
2429

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

How about fixupIsDeadOrKill ?

2447

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

2449

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?

2452

Can we assert on this assumption?

2467

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

2467
// 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?

2467

Also, please add this situation into testcase?

llvm/lib/Target/PowerPC/PPCInstrInfo.h
418

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

llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs-kill-flag.mir
5

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

27

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

81

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
2474

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.