[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

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]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

1594

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

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]

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

2 ↗

(On Diff #259549)

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

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]

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]

Simplified the mir file.

[anil9]

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]

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]

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]

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]

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.ll

 
 declare signext i32 @test3(i32 signext)
 
 declare signext i32 @test1(i32 signext)
 
 declare signext i32 @test2(i32 signext)
 
 declare signext i32 @test4(i32 signext)
+
+@b = external local_unnamed_addr global i32, align 4
+@d = external local_unnamed_addr global i32, align 4
+@c = external local_unnamed_addr global i32, align 4
+@a = external local_unnamed_addr global i32, align 4
+
+; Function Attrs: nofree norecurse nounwind
+define dso_local void @fn1() {
+; CHECK-LABEL:  fn1:
+; CHECK:  # %bb.0: # %entry
+; CHECK-NEXT:   addis r3, r2, .LC1@toc@ha
+; CHECK-NEXT:   ld r3, .LC1@toc@l(r3)
+; CHECK-NEXT:   lwz r3, 0(r3)
+; CHECK-NEXT:   cmplwi  r3, 0
+; CHECK-NEXT:   beqlr cr0
+; CHECK-NEXT:  # %bb.1: # %if.then
+; CHECK-NEXT:   addis r3, r2, .LC2@toc@ha
+; CHECK-NEXT:   addis r4, r2, .LC3@toc@ha
+; CHECK-NEXT:   addis r5, r2, .LC4@toc@ha
+; CHECK-NEXT:   ld r3, .LC2@toc@l(r3)
+; CHECK-NEXT:   ld r4, .LC3@toc@l(r4)
+; CHECK-NEXT:   ld r5, .LC4@toc@l(r5)
+; CHECK-NEXT:   lwz r3, 0(r3)
+; CHECK-NEXT:   lwz r4, 0(r4)
+; CHECK-NEXT:   cntlzw  r3, r3
+; CHECK-NEXT:   cmplwi  r4, 0
+; CHECK-NEXT:   li r4, 5
+; CHECK-NEXT:   srwi r3, r3, 5
+; CHECK-NEXT:   isel r3, r3, r4, eq
+; CHECK-NEXT:   rldic r3, r3, 2, 30
+; CHECK-NEXT:   lwax r3, r3, r5
+; CHECK-NEXT:   add r3, r3, r5
+; CHECK-NEXT:   mtctr r3
+; CHECK-NEXT:   bctr
+; CHECK-NEXT:  .LBB1_2: # %sw.bb
+; CHECK-NEXT:   addis r3, r2, .LC5@toc@ha
+; CHECK-NEXT:   li r4, 4
+; CHECK-NEXT:   ld r3, .LC5@toc@l(r3)
+; CHECK-NEXT:   stw r4, 0(r3)
+; CHECK-NEXT:  .LBB1_3: # %if.end
+; CHECK-NEXT:   blr
+
+entry:
+  %0 = load i32, i32* @c, align 4
+  %tobool3 = icmp eq i32 %0, 0
+  br i1 %tobool3, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  %1 = load i32, i32* @b, align 4
+  %tobool = icmp eq i32 %1, 0
+  %2 = load i32, i32* @d, align 4
+  %tobool1 = icmp eq i32 %2, 0
+  %cond = zext i1 %tobool1 to i32
+  %cond2 = select i1 %tobool, i32 %cond, i32 5
+  switch i32 %cond2, label %if.end6 [
+    i32 5, label %sw.bb
+    i32 0, label %sw.bb
+    i32 1, label %if.end
+    i32 4, label %if.end
+  ]
+
+sw.bb:                                            ; preds = %if.then, %if.then
+  store i32 4, i32* @a, align 4
+  br label %if.end
+
+if.end6:                                          ; preds = %if.then
+  unreachable
+
+if.end:                                           ; preds = %sw.bb, %if.then, %if.then, %entry
+  ret void
+}