This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Remove extra instruction left by emitRLDICWhenLoweringJumpTables
ClosedPublic

Authored by anil9 on Apr 13 2020, 4:19 PM.

Details

Summary

The function emitRLDICWhenLoweringJumpTables in PPCMIPeephole.cpp
was supposed to convert a pair of RLDICL and RLDICR to a single RLDIC,
but it was leaving out the RLDICL instruction. This PR fixes the bug.

Diff Detail

Unit TestsFailed

Event Timeline

anil9 created this revision.Apr 13 2020, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2020, 4:19 PM
lkail added a comment.Apr 13 2020, 9:25 PM

Could you pre-commit the test case as an NFC patch so that we can see the difference?

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
1594

I think it's safe to remove !SrcMI->hasImplicitDef() since we have known SrcMI's opcode is RLDICL and is not a record form, should not have implicit definitions.

lkail added inline comments.Apr 15 2020, 7:11 PM
llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
1594

Ooops, I made a mistake here, SrcMI might have implicit-def due to subreg. Overall LGTM.

Could you pre-commit the test case as an NFC patch so that we can see the difference?

I agree with pre-committing test cases if the codegen will change. But in this case we are simply preventing a crash in the compiler and there is not much use to a test case that just shows that the compiler crashes.

The error would be something like this

 %4:g8rc = RLDICL killed %16:g8rc, 0, 32
 %26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30

*** Bad machine code: Using a killed virtual register ***
- function:    fn1
- basic block: %bb.1 if.then (0x1000a68f270)
- instruction: %26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30
- operand 1:   %16:g8rc
fatal error: error in backend: Found 1 machine code errors.
lkail added a comment.Apr 21 2020, 9:20 PM

The error would be something like this

 %4:g8rc = RLDICL killed %16:g8rc, 0, 32
 %26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30

*** Bad machine code: Using a killed virtual register ***
- function:    fn1
- basic block: %bb.1 if.then (0x1000a68f270)
- instruction: %26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30
- operand 1:   %16:g8rc
fatal error: error in backend: Found 1 machine code errors.

Providing a mir test case would be better.

anil9 marked an inline comment as done.Apr 21 2020, 10:28 PM
anil9 added inline comments.
llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
1594

Talked with others, at this stage SrcMI is not supposed to have implicit-def due to subreg. Will be adding an assert instead.

anil9 updated this revision to Diff 259175.Apr 21 2020, 11:50 PM

Removed the check for !SrcMI->hasImplicitDef() in emitRLDICWhenLoweringJumpTables before deleting SrcMI, since at this stage we do not expect Implicit defs by SrcMI

anil9 updated this revision to Diff 259549.Apr 23 2020, 6:41 AM

Replaced the .ll test case with a .mir test case to be able to see the effect of removing SrcMI more clearly.

anil9 added a comment.Apr 23 2020, 6:49 AM

The error would be something like this

 %4:g8rc = RLDICL killed %16:g8rc, 0, 32
 %26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30

*** Bad machine code: Using a killed virtual register ***
- function:    fn1
- basic block: %bb.1 if.then (0x1000a68f270)
- instruction: %26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30
- operand 1:   %16:g8rc
fatal error: error in backend: Found 1 machine code errors.

Providing a mir test case would be better.

Done. Thanks for the suggestion :)

NeHuang added inline comments.
llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
2 ↗(On Diff #259549)

nit: I think it will be better to add a brief description for test purpose.

lkail added a comment.Apr 25 2020, 9:49 PM

Please rephrase the title by adding [PowerPC] at the front.

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
153 ↗(On Diff #259549)

What if there is a use of %4:g8rc here? Like

%30:g8rc = ADD8 %29, %28                                                                                             
%31:g8rc = ADD8 %30, %4                                                                                              
MTCTR8 %31, implicit-def $ctr8

I suppose the folding still happened and line148 turned to

%26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30

and line147 wouldn't be erased.

lkail added inline comments.Apr 25 2020, 9:50 PM
llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
3 ↗(On Diff #259549)

nit: Can add -simplify-mir to make it less noisy.

anil9 retitled this revision from Remove extra instruction left by emitRLDICWhenLoweringJumpTables to [PowerPC] Remove extra instruction left by emitRLDICWhenLoweringJumpTables.May 5 2020, 2:58 AM
anil9 updated this revision to Diff 262049.May 5 2020, 3:00 AM
anil9 set the repository for this revision to rG LLVM Github Monorepo.
anil9 marked an inline comment as done.

Simplified the mir file.

anil9 marked 2 inline comments as done.May 5 2020, 3:02 AM
anil9 added inline comments.
llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
153 ↗(On Diff #259549)

Yes that should be the case since we delete SrcMI only when there is no other nondebug use.

lkail added inline comments.May 5 2020, 3:21 AM
llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
153 ↗(On Diff #259549)

Then we still have %4:g8rc = RLDICL killed %16, 0, 32 and %26:g8rc_and_g8rc_nox0 = RLDIC %16:g8rc, 2, 30 is still using a killed vreg %16 which might not pass machine verification.

nemanjai added inline comments.May 5 2020, 3:40 AM
llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
95 ↗(On Diff #262049)

The purpose of having an mir test here is that we can specify it very concisely. This should suffice:

body:             |   
  bb.0.entry:
    liveins: $x3, $x4 
  
    %1:g8rc = COPY $x4 
    %0:g8rc = COPY $x3 
    %2:g8rc = RLDICL killed %1, 0, 32
    %3:g8rc = RLDICR %2, 2, 61
    $x3 = COPY %3
    BLR8 implicit $lr8, implicit $rm, implicit $x3

Then we should probably have two RUN lines:

  • one with -run-pass where we just check that the RLDICL is deleted
  • one with -start-before where we ensure that we don't crash and we only emit one instruction (rldic 3, 4, 2, 30)
anil9 marked an inline comment as done.May 5 2020, 3:57 AM
anil9 added inline comments.
llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
153 ↗(On Diff #259549)

But in that case the kill flag is not even supposed to exist, and if it does it is a bug with the pass that set it. And would be out of the scope of this PR to handle that.

anil9 updated this revision to Diff 262174.May 5 2020, 11:07 AM
anil9 set the repository for this revision to rG LLVM Github Monorepo.

Condensed the test case.

anil9 marked an inline comment as done.May 5 2020, 11:08 AM
nemanjai accepted this revision.May 6 2020, 2:58 AM

LGTM. Thanks for cutting down the test case.

This revision is now accepted and ready to land.May 6 2020, 2:58 AM
This revision was automatically updated to reflect the committed changes.