[llvm] r355515 - Revert "[CodeGen] Omit range checks from jump tables when lowering switches with unreachable default"

Alexander Kornienko via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 6 07:23:50 PST 2019


Author: alexfh
Date: Wed Mar  6 07:23:50 2019
New Revision: 355515

URL: http://llvm.org/viewvc/llvm-project?rev=355515&view=rev
Log:
Revert "[CodeGen] Omit range checks from jump tables when lowering switches with unreachable default"

This reverts commit 2a0f2c5ef3330846149598220467d9f3c6e8b99c (r355490).

The commit causes an assertion failure when compiling LLVM code:
$ cat repro.cpp
class QQQ {
public:
  bool x() const;
  bool y() const;
  unsigned getSizeInBits() const {
    if (y() || x())
      return getScalarSizeInBits();
    return getScalarSizeInBits() * 2;
  }
  unsigned getScalarSizeInBits() const;
};
int f(const QQQ &Ty) {
  switch (Ty.getSizeInBits()) {
    case 1:
    case 8:
      return 0;
    case 16:
      return 1;
    case 32:
      return 2;
    case 64:
      return 3;
    default:
      __builtin_unreachable();
  }
}
$ clang -O2 -o repro.o repro.cpp
assert.h assertion failed at llvm/include/llvm/ADT/ilist_iterator.h:139 in llvm::ilist_iterator::reference llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::MachineInstr, true, true, void>, true, false>::operator*() const [OptionsT = llvm::ilist_detail::node_options<llvm::MachineInstr, true, true, void>, IsReverse = true, IsConst = false]: !NodePtr->isKnownSentinel()
*** Check failure stack trace: ***
    @     0x558aab4afc10  __assert_fail
    @     0x558aa885479b  llvm::ilist_iterator<>::operator*()
    @     0x558aa8854715  llvm::MachineInstrBundleIterator<>::operator*()
    @     0x558aa92c33c3  llvm::X86InstrInfo::optimizeCompareInstr()
    @     0x558aa9a9c251  (anonymous namespace)::PeepholeOptimizer::optimizeCmpInstr()
    @     0x558aa9a9b371  (anonymous namespace)::PeepholeOptimizer::runOnMachineFunction()
    @     0x558aa99a4fc8  llvm::MachineFunctionPass::runOnFunction()
    @     0x558aab019fc4  llvm::FPPassManager::runOnFunction()
    @     0x558aab01a3a5  llvm::FPPassManager::runOnModule()
    @     0x558aab01aa9b  (anonymous namespace)::MPPassManager::runOnModule()
    @     0x558aab01a635  llvm::legacy::PassManagerImpl::run()
    @     0x558aab01afe1  llvm::legacy::PassManager::run()
    @     0x558aa5914769  (anonymous namespace)::EmitAssemblyHelper::EmitAssembly()
    @     0x558aa5910f44  clang::EmitBackendOutput()
    @     0x558aa5906135  clang::BackendConsumer::HandleTranslationUnit()
    @     0x558aa6d165ad  clang::ParseAST()
    @     0x558aa6a94e22  clang::ASTFrontendAction::ExecuteAction()
    @     0x558aa590255d  clang::CodeGenAction::ExecuteAction()
    @     0x558aa6a94840  clang::FrontendAction::Execute()
    @     0x558aa6a38cca  clang::CompilerInstance::ExecuteAction()
    @     0x558aa4e2294b  clang::ExecuteCompilerInvocation()
    @     0x558aa4df6200  cc1_main()
    @     0x558aa4e1b37f  ExecuteCC1Tool()
    @     0x558aa4e1a725  main
    @     0x7ff20d56abbd  __libc_start_main
    @     0x558aa4df51c9  _start

Removed:
    llvm/trunk/test/CodeGen/AArch64/switch-unreachable-default.ll
Modified:
    llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
    llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
    llvm/trunk/test/CodeGen/X86/pr38743.ll
    llvm/trunk/test/CodeGen/X86/switch-jump-table.ll

Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp?rev=355515&r1=355514&r2=355515&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (original)
+++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Wed Mar  6 07:23:50 2019
@@ -2388,31 +2388,24 @@ void SelectionDAGBuilder::visitJumpTable
                                     JumpTableReg, SwitchOp);
   JT.Reg = JumpTableReg;
 
-  if (!JTH.OmitRangeCheck) {
-    // Emit the range check for the jump table, and branch to the default block
-    // for the switch statement if the value being switched on exceeds the
-    // largest case in the switch.
-    SDValue CMP = DAG.getSetCC(
-        dl, TLI.getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(),
-                                   Sub.getValueType()),
-        Sub, DAG.getConstant(JTH.Last - JTH.First, dl, VT), ISD::SETUGT);
-
-    SDValue BrCond = DAG.getNode(ISD::BRCOND, dl,
-                                 MVT::Other, CopyTo, CMP,
-                                 DAG.getBasicBlock(JT.Default));
-
-    // Avoid emitting unnecessary branches to the next block.
-    if (JT.MBB != NextBlock(SwitchBB))
-      BrCond = DAG.getNode(ISD::BR, dl, MVT::Other, BrCond,
-                           DAG.getBasicBlock(JT.MBB));
- 
-    DAG.setRoot(BrCond);
-  } else {
-    SDValue BrCond = DAG.getNode(ISD::BR, dl, MVT::Other, CopyTo,
-                                 DAG.getBasicBlock(JT.MBB));
-    DAG.setRoot(BrCond);
-    SwitchBB->removeSuccessor(JT.Default, true);
-  }
+  // Emit the range check for the jump table, and branch to the default block
+  // for the switch statement if the value being switched on exceeds the largest
+  // case in the switch.
+  SDValue CMP = DAG.getSetCC(
+      dl, TLI.getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(),
+                                 Sub.getValueType()),
+      Sub, DAG.getConstant(JTH.Last - JTH.First, dl, VT), ISD::SETUGT);
+
+  SDValue BrCond = DAG.getNode(ISD::BRCOND, dl,
+                               MVT::Other, CopyTo, CMP,
+                               DAG.getBasicBlock(JT.Default));
+
+  // Avoid emitting unnecessary branches to the next block.
+  if (JT.MBB != NextBlock(SwitchBB))
+    BrCond = DAG.getNode(ISD::BR, dl, MVT::Other, BrCond,
+                         DAG.getBasicBlock(JT.MBB));
+
+  DAG.setRoot(BrCond);
 }
 
 /// Create a LOAD_STACK_GUARD node, and let it carry the target specific global
@@ -9797,13 +9790,10 @@ bool SelectionDAGBuilder::buildJumpTable
                      ->createJumpTableIndex(Table);
 
   // Set up the jump table info.
-  bool UnreachableDefault =
-      isa<UnreachableInst>(SI->getDefaultDest()->getFirstNonPHIOrDbg());
-  bool OmitRangeCheck = UnreachableDefault;
   JumpTable JT(-1U, JTI, JumpTableMBB, nullptr);
   JumpTableHeader JTH(Clusters[First].Low->getValue(),
                       Clusters[Last].High->getValue(), SI->getCondition(),
-                      nullptr, false, OmitRangeCheck);
+                      nullptr, false);
   JTCases.emplace_back(std::move(JTH), std::move(JT));
 
   JTCluster = CaseCluster::jumpTable(Clusters[First].Low, Clusters[Last].High,
@@ -10609,6 +10599,38 @@ void SelectionDAGBuilder::visitSwitch(co
   // if there are many clusters.
   sortAndRangeify(Clusters);
 
+  if (TM.getOptLevel() != CodeGenOpt::None) {
+    // Replace an unreachable default with the most popular destination.
+    // FIXME: Exploit unreachable default more aggressively.
+    bool UnreachableDefault =
+        isa<UnreachableInst>(SI.getDefaultDest()->getFirstNonPHIOrDbg());
+    if (UnreachableDefault && !Clusters.empty()) {
+      DenseMap<const BasicBlock *, unsigned> Popularity;
+      unsigned MaxPop = 0;
+      const BasicBlock *MaxBB = nullptr;
+      for (auto I : SI.cases()) {
+        const BasicBlock *BB = I.getCaseSuccessor();
+        if (++Popularity[BB] > MaxPop) {
+          MaxPop = Popularity[BB];
+          MaxBB = BB;
+        }
+      }
+      // Set new default.
+      assert(MaxPop > 0 && MaxBB);
+      DefaultMBB = FuncInfo.MBBMap[MaxBB];
+
+      // Remove cases that were pointing to the destination that is now the
+      // default.
+      CaseClusterVector New;
+      New.reserve(Clusters.size());
+      for (CaseCluster &CC : Clusters) {
+        if (CC.MBB != DefaultMBB)
+          New.push_back(CC);
+      }
+      Clusters = std::move(New);
+    }
+  }
+
   // The branch probablity of the peeled case.
   BranchProbability PeeledCaseProb = BranchProbability::getZero();
   MachineBasicBlock *PeeledSwitchMBB =

Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h?rev=355515&r1=355514&r2=355515&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h (original)
+++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h Wed Mar  6 07:23:50 2019
@@ -277,12 +277,11 @@ private:
     const Value *SValue;
     MachineBasicBlock *HeaderBB;
     bool Emitted;
-    bool OmitRangeCheck;
 
     JumpTableHeader(APInt F, APInt L, const Value *SV, MachineBasicBlock *H,
-                    bool E = false, bool ORC = false)
+                    bool E = false)
         : First(std::move(F)), Last(std::move(L)), SValue(SV), HeaderBB(H),
-          Emitted(E), OmitRangeCheck(ORC) {}
+          Emitted(E) {}
   };
   using JumpTableBlock = std::pair<JumpTableHeader, JumpTable>;
 

Removed: llvm/trunk/test/CodeGen/AArch64/switch-unreachable-default.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/switch-unreachable-default.ll?rev=355514&view=auto
==============================================================================
--- llvm/trunk/test/CodeGen/AArch64/switch-unreachable-default.ll (original)
+++ llvm/trunk/test/CodeGen/AArch64/switch-unreachable-default.ll (removed)
@@ -1,63 +0,0 @@
-; RUN: llc -O3 -o - %s | FileCheck %s
-
-; Test that the output in the presence of an unreachable default does not have
-; a compare and branch at the top of the switch to handle the default case.
-
-target triple = "aarch64-unknown-linux-gnu"
-
-; Function Attrs: nounwind
-define void @fn(i4) {
-  switch i4 %0, label %default [
-    i4 0, label %case_0
-    i4 1, label %case_1
-    i4 2, label %case_2
-    i4 3, label %case_3
-    i4 4, label %case_4
-    i4 5, label %case_5
-  ]
-
-; CHECK-LABEL: fn:
-; CHECK-NOT:    sub
-; CHECK-NOT:    cmp
-; CHECK-NOT:    b.hi
-; CHECK:        ldrb {{w[0-9]+}}, [{{x[0-9]+}}, {{x[0-9]+}}]
-; CHECK:        add {{x[0-9]+}}, {{x[0-9]+}}, {{x[0-9]+}}, lsl #2
-; CHECK:        br {{x[0-9]+}}
-
-default:
-  unreachable
-
-case_0:
-  tail call void @handle_case_00(i4 %0) #2
-  br label %return_label
-
-case_1:
-  tail call void @handle_case_01(i4 %0) #2
-  br label %return_label
-
-case_2:
-  tail call void @handle_case_02(i4 %0) #2
-  br label %return_label
-
-case_3:
-  tail call void @handle_case_03(i4 %0) #2
-  br label %return_label
-
-case_4:
-  tail call void @handle_case_04(i4 %0) #2
-  br label %return_label
-
-case_5:
-  tail call void @handle_case_05(i4 %0) #2
-  br label %return_label
-
-return_label:
-  ret void
-}
-
-declare  void @handle_case_00(i4)
-declare  void @handle_case_01(i4)
-declare  void @handle_case_02(i4)
-declare  void @handle_case_03(i4)
-declare  void @handle_case_04(i4)
-declare  void @handle_case_05(i4)

Modified: llvm/trunk/test/CodeGen/X86/pr38743.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/pr38743.ll?rev=355515&r1=355514&r2=355515&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/X86/pr38743.ll (original)
+++ llvm/trunk/test/CodeGen/X86/pr38743.ll Wed Mar  6 07:23:50 2019
@@ -18,43 +18,41 @@ declare void @llvm.memcpy.p0i8.p0i8.i64(
 
 define void @pr38743() #1 align 2 {
 ; CHECK-LABEL: pr38743:
-; CHECK:       # %bb.0:                                # %bb
-; CHECK-NEXT:  	xorl	%eax, %eax
-; CHECK-NEXT:  	jmpq	*.LJTI0_0(,%rax,8)
-; CHECK-NEXT: .[[LABEL1:[A-Z_0-9]+]]:                                # %bb5
-; CHECK-NEXT: 	movzwl	.str.17+{{.*}}(%rip), %eax
-; CHECK-NEXT: 	movw	%ax, -{{[0-9]+}}(%rsp)
-; CHECK-NEXT: 	movq	.str.17(%rip), %rax
-; CHECK-NEXT: 	jmp	.[[LABEL4:[A-Z_0-9]+]]
-; CHECK-NEXT: .[[LABEL2:[A-Z_0-9]+]]:                                # %bb2
-; CHECK-NEXT: 	movq	.str.16+{{.*}}(%rip), %rax
-; CHECK-NEXT: 	movq	%rax, -{{[0-9]+}}(%rsp)
-; CHECK-NEXT: 	movq	.str.16(%rip), %rax
-; CHECK-NEXT: 	jmp	.[[LABEL4]]
-; CHECK-NEXT: .[[LABEL3:[A-Z_0-9]+]]:                                # %bb8
-; CHECK-NEXT: 	movq	.str.18+{{.*}}(%rip), %rax
-; CHECK-NEXT: 	movq	%rax, -{{[0-9]+}}(%rsp)
-; CHECK-NEXT: 	movq	.str.18(%rip), %rax
-; CHECK-NEXT: .[[LABEL4]]:                                # %bb12
-; CHECK-NEXT: 	movq	%rax, -{{[0-9]+}}(%rsp)
-; CHECK-NEXT: 	movq	-{{[0-9]+}}(%rsp), %rax
-; CHECK-NEXT: 	movq	%rax, (%rax)
-; CHECK-NEXT: 	movb	-{{[0-9]+}}(%rsp), %al
-; CHECK-NEXT: 	movq	-{{[0-9]+}}(%rsp), %rcx
-; CHECK-NEXT: 	movzwl	-{{[0-9]+}}(%rsp), %edx
-; CHECK-NEXT: 	movl	-{{[0-9]+}}(%rsp), %esi
-; CHECK-NEXT: 	movb	-{{[0-9]+}}(%rsp), %dil
-; CHECK-NEXT: 	movb	%al, (%rax)
-; CHECK-NEXT: 	movq	%rcx, {{[0-9]+}}(%rax)
-; CHECK-NEXT: 	movw	%dx, {{[0-9]+}}(%rax)
-; CHECK-NEXT: 	movl	%esi, {{[0-9]+}}(%rax)
-; CHECK-NEXT: 	movb	%dil, {{[0-9]+}}(%rax)
-; CHECK-NEXT: 	retq
-; CHECK-LABEL: .LJTI0_0:
-; CHECK:      	.quad	.[[LABEL2]]
-; CHECK-NEXT: 	.quad	.[[LABEL1]]
-; CHECK-NEXT: 	.quad	.[[LABEL3]]
-; CHECK-NEXT: 	.quad	.[[LABEL1]]
+; CHECK:       # %bb.0: # %bb
+; CHECK-NEXT:    cmpl $3, %eax
+; CHECK-NEXT:    je .LBB0_4
+; CHECK-NEXT:  # %bb.1: # %bb
+; CHECK-NEXT:    cmpl $1, %eax
+; CHECK-NEXT:    je .LBB0_2
+; CHECK-NEXT:  # %bb.3: # %bb5
+; CHECK-NEXT:    movzwl .str.17+{{.*}}(%rip), %eax
+; CHECK-NEXT:    movw %ax, -{{[0-9]+}}(%rsp)
+; CHECK-NEXT:    movq {{.*}}(%rip), %rax
+; CHECK-NEXT:    jmp .LBB0_5
+; CHECK-NEXT:  .LBB0_4: # %bb8
+; CHECK-NEXT:    movq .str.18+{{.*}}(%rip), %rax
+; CHECK-NEXT:    movq %rax, -{{[0-9]+}}(%rsp)
+; CHECK-NEXT:    movq {{.*}}(%rip), %rax
+; CHECK-NEXT:    jmp .LBB0_5
+; CHECK-NEXT:  .LBB0_2: # %bb2
+; CHECK-NEXT:    movq .str.16+{{.*}}(%rip), %rax
+; CHECK-NEXT:    movq %rax, -{{[0-9]+}}(%rsp)
+; CHECK-NEXT:    movq {{.*}}(%rip), %rax
+; CHECK-NEXT:  .LBB0_5: # %bb12
+; CHECK-NEXT:    movq %rax, -{{[0-9]+}}(%rsp)
+; CHECK-NEXT:    movq -{{[0-9]+}}(%rsp), %rax
+; CHECK-NEXT:    movq %rax, (%rax)
+; CHECK-NEXT:    movb -{{[0-9]+}}(%rsp), %al
+; CHECK-NEXT:    movq -{{[0-9]+}}(%rsp), %rcx
+; CHECK-NEXT:    movzwl -{{[0-9]+}}(%rsp), %edx
+; CHECK-NEXT:    movl -{{[0-9]+}}(%rsp), %esi
+; CHECK-NEXT:    movb -{{[0-9]+}}(%rsp), %dil
+; CHECK-NEXT:    movb %al, (%rax)
+; CHECK-NEXT:    movq %rcx, 1(%rax)
+; CHECK-NEXT:    movw %dx, 9(%rax)
+; CHECK-NEXT:    movl %esi, 11(%rax)
+; CHECK-NEXT:    movb %dil, 15(%rax)
+; CHECK-NEXT:    retq
 bb:
   %tmp = alloca %0, align 16
   %tmp1 = bitcast %0* %tmp to i8*

Modified: llvm/trunk/test/CodeGen/X86/switch-jump-table.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/switch-jump-table.ll?rev=355515&r1=355514&r2=355515&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/X86/switch-jump-table.ll (original)
+++ llvm/trunk/test/CodeGen/X86/switch-jump-table.ll Wed Mar  6 07:23:50 2019
@@ -2,12 +2,14 @@
 ; RUN: llc -mtriple=i686-pc-gnu-linux -print-machineinstrs=expand-isel-pseudos %s -o /dev/null 2>&1 | FileCheck %s -check-prefix=CHECK-JT-PROB
 
 
-; An unreachable default destination is ignored and no compare and branch
-; is generated for the default values.
+; An unreachable default destination is replaced with the most popular case label.
 
 define void @foo(i32 %x, i32* %to) {
 ; CHECK-LABEL: foo:
 ; CHECK: movl 4(%esp), [[REG:%e[a-z]{2}]]
+; CHECK: cmpl $3, [[REG]]
+; CHECK: ja .LBB0_6
+; CHECK-NEXT: # %bb.1:
 ; CHECK-NEXT: jmpl *.LJTI0_0(,[[REG]],4)
 ; CHECK: movl $4
 ; CHECK: retl
@@ -43,12 +45,10 @@ default:
 
 ; The jump table has four entries.
 ; CHECK-LABEL: .LJTI0_0:
-; CHECK-NEXT: .long  .LBB0_1
 ; CHECK-NEXT: .long  .LBB0_2
 ; CHECK-NEXT: .long  .LBB0_3
 ; CHECK-NEXT: .long  .LBB0_4
 ; CHECK-NEXT: .long  .LBB0_5
-; CHECK-NEXT: .long  .LBB0_5
 }
 
 ; Check if branch probabilities are correctly assigned to the jump table.




More information about the llvm-commits mailing list