Page MenuHomePhabricator

[PowerPC] Remove redundant CRSET/CRUNSET in custom lowering of known CR bit spills
ClosedPublic

Authored by Yi-Hong.Lyu on Sep 18 2019, 10:41 AM.

Details

Summary

We lower known CR bit spills (CRSET/CRUNSET) to load and spill the known value
but forgot to remove the redundant spills.

e.g., This sequence was used to spill a CRUNSET:

crclr   4*cr5+lt
mfocrf  r3,4
rlwinm  r3,r3,20,0,0
stw     r3,132(r1)

Custom lowering of known CR bit spills lower it to:

crclr   4*cr5+lt
li        r3,0
stw    r3,132(r1)

crxor is redundant if there is no use of 4*cr5+lt so we should remove it

Diff Detail

Event Timeline

Yi-Hong.Lyu created this revision.Sep 18 2019, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 10:41 AM
Yi-Hong.Lyu edited the summary of this revision. (Show Details)Sep 18 2019, 10:48 AM
Yi-Hong.Lyu edited the summary of this revision. (Show Details)Sep 18 2019, 10:48 AM
jsji added a reviewer: Restricted Project.Sep 18 2019, 11:23 AM
amyk added a subscriber: amyk.Sep 26 2019, 12:36 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
759

Maybe we can add an extra comment here, like how there is a comment above the modifiesRegister() call?

Address Amy's comment

Yi-Hong.Lyu marked an inline comment as done.Sep 26 2019, 9:07 PM
nemanjai requested changes to this revision.Oct 21 2019, 7:57 PM

I believe we could end up with run-time errors (SIGILL) if we get rid of a CRBIT spill and build with -ppc-late-peephole=false. As a result, we need to make sure we get rid of any of those nops we may have introduced.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
3180

Please add a comment here along the lines of:

// Pseudo-instruction marked for deletion. When deleting the instruction
// would cause iterator invalidation in MIR transformation passes, this
// pseudo can be used instead. It will be removed unconditionally at
// pre-emit time (prior to branch selection).
llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp
164–172

We need to change this to add a loop that only gets rid of UNENCODED_NOP instructions. Regardless of whether we want any of the transformations in this pass to run, we definitely need to get rid of those. If we are skipping the rest of the transformations, then we just need to get rid of these nops.

This revision now requires changes to proceed.Oct 21 2019, 7:57 PM

Address Nemanja's comments

Yi-Hong.Lyu marked 2 inline comments as done.Oct 24 2019, 1:07 PM
nemanjai accepted this revision.Nov 1 2019, 4:46 AM

LGTM. Feel free to address the comments on the commit.

llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp
165

Nit: Expand this comment to:

// Remove UNENCODED_NOP even when this pass is disabled.
// This needs to be done unconditionally so we don't emit zeros
// in the instruction stream.
llvm/test/CodeGen/PowerPC/knowCRBitSpill.ll
6

Please add --implicit-check-not=creqv --implicit-check-not=crxor as we want to make sure we don't emit the CRSET/CRUNSET.

This revision is now accepted and ready to land.Nov 1 2019, 4:46 AM
Yi-Hong.Lyu closed this revision.Fri, Nov 8, 7:38 AM

Pushed here https://github.com/llvm/llvm-project/commit/a3db9c08ebdf1f39ed89f4a7afa09fc153cf98c5. Close it manually since I misuse r as in Differential Revision.

BTW, all comments are addressed and updated accordingly before push