[ISEL][BitTestBlock] pre-commit test for D109103

[nickdesaulniers]

Summary

Upload a test that shows ISEL taking a SwitchInst that has an
unreachable BB for a default target being lowered to an unconditional
jump off the end of a function.

Link: https://bugs.llvm.org/show_bug.cgi?id=50080
Link: https://github.com/ClangBuiltLinux/linux/issues/679
Link: https://github.com/ClangBuiltLinux/linux/issues/1440

[lebedev.ri]

No need for reviewing this, just commit :)

[nickdesaulniers]

In D109106#2978265, @lebedev.ri wrote:

No need for reviewing this, just commit :)

Perhaps, but if D109103 is not accepted, then it might not be adding anything interesting to the existing test coverage. I'll wait to see how the review of D109103 before committing, then push them out together.

[hans]

llvm/test/CodeGen/X86/SwitchLowering.ll

78

I guess it doesn't make sense if you push them together, but if you want to pre-commit this now, I'd put a "FIXME: Get rid of this conditional jump and bit test, see PRxxx".

[nickdesaulniers]

• add llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll from D109103, add -global-isel=1 coverage.

[nickdesaulniers]

• remember to git add new test ;)

[nickdesaulniers]

llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll

7

Is adding this much MIR withought llvm/utils/update_mir_test_checks.py being runable frowned upon?

llvm/utils/update_mir_test_checks.py doesn't work for this test, as written.

[nickdesaulniers]

• add fixme to other test

[hans]

llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll

7

I don't know, but why are you doing MIR checks instead of the final assembly, which should be less verbose and therefore hopefully more robust?

[nickdesaulniers]

llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll

7

Fair question.

IMO ISEL is pass that makes the decisions here that are relevant, so ISEL is the unit that should be tested.

Running all backend passes is an integration test of multiple passes, and is what llvm/test/CodeGen/X86/SwitchLowering.ll does. So we have an existing integration test of all backend passes with: llvm/test/CodeGen/X86/SwitchLowering.ll, and newly added unit tests of ISEL with llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll.

Would you prefer I convert llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll to check ASM rather than MIR? Do other reviewers agree?

[craig.topper]

llvm/test/CodeGen/X86/SwitchLowering.ll

67

predecessor*

llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll

4

I think SDISEL would be a better abbreviation. It's used in other places in the codebase

7

Is the empty basic block guaranteed to be at the end of the function in the ASM output. That's the only way to demonstrate the bug from the ASM I think. The MIR shows the predecessor and successor lists which makes it more obvious.

[nickdesaulniers]

• s/SISEL/SDISEL/g

[nickdesaulniers]

• s/predacessor/predecessor/g

[nickdesaulniers]

llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll

7

Is the empty basic block guaranteed to be at the end of the function in the ASM output.

For this input, it happens to be. I don't know about guaranteed though.

The MIR shows the predecessor and successor lists which makes it more obvious.

So +2 then? :)

[craig.topper]

LGTM

[hans]

lgtm

llvm/test/CodeGen/X86/SwitchLowering.ll

 
 bb7:            ; preds = %bb, %bb
         tail call void @foo( i8 %tmp )
         ret i8* %tmp2
 }
 
 declare void @foo(i8)
 
+define i32 @baz(i32 %0) {
+; FIXME: Get rid of this conditional jump and bit test in .LBB1_1.
+; FIXME: .LBB1_4 should not have .LBB1_1 as a predecessor, or be past the end
+; FIXME: of the function.
+; CHECK-LABEL: baz:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %ecx
+; CHECK-NEXT:    movl $13056, %edx # imm = 0x3300
+; CHECK-NEXT:    btl %ecx, %edx
+; CHECK-NEXT:    jae .LBB1_1
+; CHECK-NEXT:  # %bb.3: # %return
+; CHECK-NEXT:    retl
+; CHECK-NEXT:  .LBB1_1:
+; CHECK-NEXT:    movl $48, %eax
+; CHECK-NEXT:    btl %ecx, %eax
+; CHECK-NEXT:    jae .LBB1_4
+; CHECK-NEXT:  # %bb.2: # %sw.epilog8
+; CHECK-NEXT:    movl $1, %eax
+; CHECK-NEXT:    retl
+; CHECK-NEXT:  .LBB1_4: # %if.then.unreachabledefault
+  switch i32 %0, label %if.then.unreachabledefault [
+    i32 4, label %sw.epilog8
+    i32 5, label %sw.epilog8
+    i32 8, label %sw.bb2
+    i32 9, label %sw.bb2
+    i32 12, label %sw.bb4
+    i32 13, label %sw.bb4
+  ]
+
+sw.bb2:
+  br label %return
+
+sw.bb4:
+  br label %return
+
+sw.epilog8:
+  br label %return
+
+if.then.unreachabledefault:
+  unreachable
+
+return:
+  %retval.0 = phi i32 [ 1, %sw.epilog8 ], [ 0, %sw.bb2 ], [ 0, %sw.bb4 ]
+  ret i32 %retval.0
+}

llvm/test/CodeGen/X86/switch-bit-test-unreachable-default.ll

+; RUN: llc -mtriple=x86_64-- -global-isel=0 -print-after=finalize-isel \
+; RUN:   -stop-after=finalize-isel %s -o /dev/null 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=CHECK-SDISEL
+; RUN: llc -mtriple=x86_64-- -global-isel=1 -print-after=finalize-isel \
+; RUN:   -stop-after=finalize-isel %s -o /dev/null 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=CHECK-GISEL
+
+; PR50080
+define i32 @baz(i32 %0) {
+; FIXME: Get rid of this conditional jump and bit test in bb.5.
+; FIXME: bb.2 should not have bb.5 as a predecessor.
+; CHECK-SDISEL: bb.0 (%ir-block.1):
+; CHECK-SDISEL:   successors: %bb.4(0x80000000); %bb.4(100.00%)
+; CHECK-SDISEL:   liveins: $edi
+; CHECK-SDISEL:   %1:gr32 = COPY $edi
+; CHECK-SDISEL:   %2:gr32 = MOV32r0 implicit-def dead $eflags
+; CHECK-SDISEL:   %3:gr32 = COPY %1:gr32
+; CHECK-SDISEL: bb.4 (%ir-block.1):
+; CHECK-SDISEL: ; predecessors: %bb.0
+; CHECK-SDISEL:   successors: %bb.3(0x55555555), %bb.5(0x2aaaaaab); %bb.3(66.67%), %bb.5(33.33%)
+; CHECK-SDISEL:   %4:gr32 = MOV32ri 13056
+; CHECK-SDISEL:   BT32rr killed %4:gr32, %3:gr32, implicit-def $eflags
+; CHECK-SDISEL:   JCC_1 %bb.3, 2, implicit $eflags
+; CHECK-SDISEL: bb.5 (%ir-block.1):
+; CHECK-SDISEL: ; predecessors: %bb.4
+; CHECK-SDISEL:   successors: %bb.1(0x80000000), %bb.2(0x00000000); %bb.1(100.00%), %bb.2(0.00%)
+; CHECK-SDISEL:   %5:gr32 = MOV32ri 48
+; CHECK-SDISEL:   BT32rr killed %5:gr32, %3:gr32, implicit-def $eflags
+; CHECK-SDISEL:   JCC_1 %bb.1, 2, implicit $eflags
+; CHECK-SDISEL:   JMP_1 %bb.2
+; CHECK-SDISEL: bb.1.sw.epilog8:
+; CHECK-SDISEL: ; predecessors: %bb.5
+; CHECK-SDISEL:   successors: %bb.3(0x80000000); %bb.3(100.00%)
+; CHECK-SDISEL:   %6:gr32 = MOV32ri 1
+; CHECK-SDISEL:   JMP_1 %bb.3
+; CHECK-SDISEL: bb.2.if.then.unreachabledefault:
+; CHECK-SDISEL: ; predecessors: %bb.5
+; CHECK-SDISEL: bb.3.return:
+; CHECK-SDISEL: ; predecessors: %bb.4, %bb.1
+; CHECK-SDISEL:   %0:gr32 = PHI %2:gr32, %bb.4, %6:gr32, %bb.1
+; CHECK-SDISEL:   $eax = COPY %0:gr32
+; CHECK-SDISEL:   RET 0, $eax
+
+
+; FIXME: Get rid of this conditional jump and bit test in bb.6.
+; FIXME: bb.3 should not have bb.6 as a predecessor.
+; CHECK-GISEL: bb.1 (%ir-block.1):
+; CHECK-GISEL:   successors: %bb.5(0x80000000); %bb.5(100.00%)
+; CHECK-GISEL:   liveins: $edi
+; CHECK-GISEL:   %0:gr32 = COPY $edi
+; CHECK-GISEL:   %16:gr32 = MOV32ri 1
+; CHECK-GISEL:   %17:gr32 = MOV32r0 implicit-def $eflags
+; CHECK-GISEL:   %2:gr32 = SUB32ri8 %0:gr32(tied-def 0), 0, implicit-def $eflags
+; CHECK-GISEL: bb.5 (%ir-block.1):
+; CHECK-GISEL: ; predecessors: %bb.1
+; CHECK-GISEL:   successors: %bb.4(0x55555555), %bb.6(0x2aaaaaab); %bb.4(66.67%), %bb.6(33.33%)
+; CHECK-GISEL:   %3:gr32 = MOV32ri 1
+; CHECK-GISEL:   %21:gr8 = COPY %2.sub_8bit:gr32
+; CHECK-GISEL:   $cl = COPY %21:gr8
+; CHECK-GISEL:   %4:gr32 = SHL32rCL %3:gr32(tied-def 0), implicit-def $eflags, implicit $cl
+; CHECK-GISEL:   %6:gr32 = AND32ri %4:gr32(tied-def 0), 13056, implicit-def $eflags
+; CHECK-GISEL:   %7:gr32 = MOV32r0 implicit-def $eflags
+; CHECK-GISEL:   CMP32rr %6:gr32, %7:gr32, implicit-def $eflags
+; CHECK-GISEL:   %20:gr8 = SETCCr 5, implicit $eflags
+; CHECK-GISEL:   TEST8ri %20:gr8, 1, implicit-def $eflags
+; CHECK-GISEL:   JCC_1 %bb.4, 5, implicit $eflags
+; CHECK-GISEL: bb.6 (%ir-block.1):
+; CHECK-GISEL: ; predecessors: %bb.5
+; CHECK-GISEL:   successors: %bb.2(0x80000000), %bb.3(0x00000000); %bb.2(100.00%), %bb.3(0.00%)
+; CHECK-GISEL:   %9:gr32 = MOV32ri 1
+; CHECK-GISEL:   %19:gr8 = COPY %2.sub_8bit:gr32
+; CHECK-GISEL:   $cl = COPY %19:gr8
+; CHECK-GISEL:   %10:gr32 = SHL32rCL %9:gr32(tied-def 0), implicit-def $eflags, implicit $cl
+; CHECK-GISEL:   %12:gr32 = AND32ri8 %10:gr32(tied-def 0), 48, implicit-def $eflags
+; CHECK-GISEL:   %13:gr32 = MOV32r0 implicit-def $eflags
+; CHECK-GISEL:   CMP32rr %12:gr32, %13:gr32, implicit-def $eflags
+; CHECK-GISEL:   %18:gr8 = SETCCr 5, implicit $eflags
+; CHECK-GISEL:   TEST8ri %18:gr8, 1, implicit-def $eflags
+; CHECK-GISEL:   JCC_1 %bb.2, 5, implicit $eflags
+; CHECK-GISEL:   JMP_1 %bb.3
+; CHECK-GISEL: bb.2.sw.epilog8:
+; CHECK-GISEL: ; predecessors: %bb.6
+; CHECK-GISEL:   successors: %bb.4(0x80000000); %bb.4(100.00%)
+; CHECK-GISEL:   JMP_1 %bb.4
+; CHECK-GISEL: bb.3.if.then.unreachabledefault:
+; CHECK-GISEL: ; predecessors: %bb.6
+; CHECK-GISEL: bb.4.return:
+; CHECK-GISEL: ; predecessors: %bb.5, %bb.2
+; CHECK-GISEL:   %15:gr32 = PHI %16:gr32, %bb.2, %17:gr32, %bb.5
+; CHECK-GISEL:   $eax = COPY %15:gr32
+; CHECK-GISEL:   RET 0, implicit $eax
+
+  switch i32 %0, label %if.then.unreachabledefault [
+    i32 4, label %sw.epilog8
+    i32 5, label %sw.epilog8
+    i32 8, label %sw.bb2
+    i32 9, label %sw.bb2
+    i32 12, label %sw.bb4
+    i32 13, label %sw.bb4
+  ]
+
+sw.bb2:
+  br label %return
+
+sw.bb4:
+  br label %return
+
+sw.epilog8:
+  br label %return
+
+if.then.unreachabledefault:
+  unreachable
+
+return:
+  %retval.0 = phi i32 [ 1, %sw.epilog8 ], [ 0, %sw.bb2 ], [ 0, %sw.bb4 ]
+  ret i32 %retval.0
+}