[PowerPC] Remove extra instruction left by emitRLDICWhenLoweringJumpTables

[anil9]

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.

[lkail]

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

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1569

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]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1569

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

[anil9]

In D78063#1979659, @lkail wrote:

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]

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]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1569

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

[anil9]

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

[anil9]

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

[anil9]

In D78063#1995983, @lkail wrote:

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]

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

3

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

[lkail]

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

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

154

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]

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

4

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

[anil9]

Simplified the mir file.

[anil9]

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

154

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

[lkail]

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

154

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]

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

96

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]

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

154

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]

Condensed the test case.

[nemanjai]

LGTM. Thanks for cutting down the test case.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

bool PPCMIPeephole::emitRLDICWhenLoweringJumpTables(MachineInstr &MI) {
   MI.setDesc(TII->get(PPC::RLDIC));
   MI.getOperand(1).setReg(SrcMI->getOperand(1).getReg());
   MI.getOperand(2).setImm(NewSH);
   MI.getOperand(3).setImm(NewMB);
 
   LLVM_DEBUG(dbgs() << "To: ");
   LLVM_DEBUG(MI.dump());
   NumRotatesCollapsed++;
+  // If SrcReg has no non-debug use it's safe to delete its def SrcMI.
+  if (MRI->use_nodbg_empty(SrcReg)) {
+    assert(!SrcMI->hasImplicitDef() &&
+           "Not expecting an implicit def with this instr.");
+    SrcMI->eraseFromParent();
+  }
   return true;
 }
 
 // For case in LLVM IR
 // entry:
 //   %iconv = sext i32 %index to i64
 //   br i1 undef label %true, label %false
 // true:

llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir

+# RUN: llc -mtriple=powerpc64le--linux-gnu -start-before ppc-mi-peepholes %s -o - -verify-machineinstrs | FileCheck %s
+# RUN: llc -mtriple=powerpc64le--linux-gnu -run-pass ppc-mi-peepholes %s -o - -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-PASS
+
+# The differential revision https://reviews.llvm.org/D60402 was supposed to
+# collapse RLDICL/RLDICR into RLDIC when possible, but it missed removing the
+# RLDICL instruction. This test case tests the fix for the bug.
+
+--- |
+  ; ModuleID = 'jump-tables-collapse-rotate-remove-SrcMI.ll'
+  source_filename = "jump-tables-collapse-rotate-remove-SrcMI.ll"
+  target datalayout = "e-m:e-i64:64-n32:64"
+
+  define dso_local i64 @test(i64 %a, i64 %b) local_unnamed_addr {
+  entry:
+    %add = add nsw i64 %b, %a
+    ret i64 %add
+  }
+
+...
+---
+name:            test
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: g8rc }
+  - { id: 1, class: g8rc }
+  - { id: 2, class: g8rc }
+liveins:
+  - { reg: '$x3', virtual-reg: '%0' }
+  - { reg: '$x4', virtual-reg: '%1' }
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+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
+
+...
+# CHECK-LABEL: test:
+# CHECK: # %bb.0: # %entry
+# CHECK-NEXT:   rldic 3, 4, 2, 30
+# CHECK-NEXT:   blr
+#
+# CHECK-PASS-NOT:     %2:g8rc = RLDICL killed %1, 0, 32
+# CHECK-PASS-NOT:     %3:g8rc = RLDICR %2, 2, 61
+# CHECK-PASS:     %3:g8rc = RLDIC %1, 2, 30