[DirectX] Resolve unreachable default branches in switch statements#193592
Conversation
Add a legalization to resolve unreachable default branches in switch statements. These are introduced when the CorrelatedValuePropagationPass proves all cases of a switch are covered, leaving the default as an unreachable block. The default destination is replaced with either the common successor block or the first switch case destination.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-backend-directx Author: Finn Plummer (inbelic) ChangesAdd a legalization to resolve unreachable default branches in switch statements. These are introduced when the The default destination is replaced with either the common successor block or the first switch case destination. This was discovered from the following case: https://godbolt.org/z/W7zE1GMT6, where we can see that the unrolling of the outer loop is not successful and the switch statement with an Assisted by: Claude Opus 4.6 Full diff: https://github.com/llvm/llvm-project/pull/193592.diff 2 Files Affected:
diff --git a/llvm/lib/Target/DirectX/DXILLegalizePass.cpp b/llvm/lib/Target/DirectX/DXILLegalizePass.cpp
index 61b10574cb506..f22da6a27ca18 100644
--- a/llvm/lib/Target/DirectX/DXILLegalizePass.cpp
+++ b/llvm/lib/Target/DirectX/DXILLegalizePass.cpp
@@ -406,6 +406,57 @@ legalizeGetHighLowi64Bytes(Instruction &I,
return false;
}
+static bool
+resolveUnreachableSwitchDefault(Instruction &I,
+ SmallVectorImpl<Instruction *> &ToRemove,
+ DenseMap<Value *, Value *> &) {
+ auto *SI = dyn_cast<SwitchInst>(&I);
+ if (!SI || SI->getNumCases() == 0)
+ return false;
+
+ BasicBlock *DefaultBB = SI->getDefaultDest();
+
+ // Check if the default destination only contains an unreachable instruction.
+ if (DefaultBB->size() != 1 ||
+ !isa<UnreachableInst>(DefaultBB->getTerminator()))
+ return false;
+
+ // Try to find a common successor of all case destinations. If all case
+ // blocks unconditionally branch to the same block, that is the common
+ // successor. This is just a best effort, and is done as the original form of
+ // the switch statement was likely in this form before being transformed to
+ // an unreachable branch.
+ BasicBlock *CommonSuccessor = nullptr;
+ for (auto &Case : SI->cases()) {
+ BasicBlock *CaseBB = Case.getCaseSuccessor();
+ auto *BI = dyn_cast<BranchInst>(CaseBB->getTerminator());
+ if (!BI || !BI->isUnconditional()) {
+ CommonSuccessor = nullptr;
+ break;
+ }
+ BasicBlock *Succ = BI->getSuccessor(0);
+ if (!CommonSuccessor)
+ CommonSuccessor = Succ;
+ else if (CommonSuccessor != Succ) {
+ CommonSuccessor = nullptr;
+ break;
+ }
+ }
+
+ BasicBlock *NewDefault =
+ CommonSuccessor ? CommonSuccessor : SI->case_begin()->getCaseSuccessor();
+
+ BasicBlock *SwitchBB = SI->getParent();
+ SI->setDefaultDest(NewDefault);
+
+ // Ensure all phi nodes are legal by adding an incoming poison value from the
+ // unreachable branch.
+ for (PHINode &Phi : NewDefault->phis())
+ Phi.addIncoming(PoisonValue::get(Phi.getType()), SwitchBB);
+
+ return true;
+}
+
static bool
legalizeScalarLoadStoreOnArrays(Instruction &I,
SmallVectorImpl<Instruction *> &ToRemove,
@@ -501,6 +552,7 @@ class DXILLegalizationPipeline {
LegalizationPipeline[Stage2].push_back(
downcastI64toI32InsertExtractElements);
LegalizationPipeline[Stage2].push_back(legalizeScalarLoadStoreOnArrays);
+ LegalizationPipeline[Stage2].push_back(resolveUnreachableSwitchDefault);
}
};
diff --git a/llvm/test/CodeGen/DirectX/legalize-switch-unreachable.ll b/llvm/test/CodeGen/DirectX/legalize-switch-unreachable.ll
new file mode 100644
index 0000000000000..3e23e3dd0bdb2
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/legalize-switch-unreachable.ll
@@ -0,0 +1,143 @@
+; RUN: opt -S -passes='dxil-legalize' -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+
+; Test that a switch with an unreachable default and a common successor across
+; all case blocks has its default redirected to the common successor.
+
+define i32 @test_common_successor(i32 %val) {
+; CHECK-LABEL: define i32 @test_common_successor(
+; CHECK: entry:
+; CHECK-NEXT: switch i32 %val, label %merge [
+; CHECK-NEXT: i32 0, label %case0
+; CHECK-NEXT: i32 1, label %case1
+; CHECK-NEXT: i32 2, label %case2
+; CHECK-NEXT: ]
+; CHECK: case0:
+; CHECK-NEXT: br label %merge
+; CHECK: case1:
+; CHECK-NEXT: br label %merge
+; CHECK: case2:
+; CHECK-NEXT: br label %merge
+; CHECK: merge:
+; CHECK-NEXT: %result = phi i32 [ 10, %case0 ], [ 20, %case1 ], [ 30, %case2 ], [ poison, %entry ]
+; CHECK-NEXT: ret i32 %result
+;
+entry:
+ switch i32 %val, label %default [
+ i32 0, label %case0
+ i32 1, label %case1
+ i32 2, label %case2
+ ]
+
+default:
+ unreachable
+
+case0:
+ br label %merge
+
+case1:
+ br label %merge
+
+case2:
+ br label %merge
+
+merge:
+ %result = phi i32 [ 10, %case0 ], [ 20, %case1 ], [ 30, %case2 ]
+ ret i32 %result
+}
+
+; Test that a switch with an unreachable default and no common successor
+; has its default redirected to the first case block.
+
+define i32 @test_no_common_successor(i32 %val) {
+; CHECK-LABEL: define i32 @test_no_common_successor(
+; CHECK: entry:
+; CHECK-NEXT: switch i32 %val, label %case0 [
+; CHECK-NEXT: i32 0, label %case0
+; CHECK-NEXT: i32 1, label %case1
+; CHECK-NEXT: ]
+; CHECK: case0:
+; CHECK-NEXT: ret i32 10
+; CHECK: case1:
+; CHECK-NEXT: ret i32 20
+;
+entry:
+ switch i32 %val, label %default [
+ i32 0, label %case0
+ i32 1, label %case1
+ ]
+
+default:
+ unreachable
+
+case0:
+ ret i32 10
+
+case1:
+ ret i32 20
+}
+
+; Test that a switch with a reachable default is not modified.
+
+define i32 @test_reachable_default(i32 %val) {
+; CHECK-LABEL: define i32 @test_reachable_default(
+; CHECK: entry:
+; CHECK-NEXT: switch i32 %val, label %default [
+; CHECK-NEXT: i32 0, label %case0
+; CHECK-NEXT: ]
+; CHECK: default:
+; CHECK-NEXT: ret i32 -1
+; CHECK: case0:
+; CHECK-NEXT: ret i32 10
+;
+entry:
+ switch i32 %val, label %default [
+ i32 0, label %case0
+ ]
+
+default:
+ ret i32 -1
+
+case0:
+ ret i32 10
+}
+
+; Test with conditional branches in case blocks (no common successor) and
+; the default falling back to first case.
+
+define i32 @test_conditional_case_blocks(i32 %val, i1 %cond) {
+; CHECK-LABEL: define i32 @test_conditional_case_blocks(
+; CHECK: entry:
+; CHECK-NEXT: switch i32 %val, label %case0 [
+; CHECK-NEXT: i32 0, label %case0
+; CHECK-NEXT: i32 1, label %case1
+; CHECK-NEXT: ]
+; CHECK: case0:
+; CHECK-NEXT: br i1 %cond, label %merge_a, label %merge_b
+; CHECK: case1:
+; CHECK-NEXT: br label %merge_a
+; CHECK: merge_a:
+; CHECK-NEXT: ret i32 1
+; CHECK: merge_b:
+; CHECK-NEXT: ret i32 2
+;
+entry:
+ switch i32 %val, label %default [
+ i32 0, label %case0
+ i32 1, label %case1
+ ]
+
+default:
+ unreachable
+
+case0:
+ br i1 %cond, label %merge_a, label %merge_b
+
+case1:
+ br label %merge_a
+
+merge_a:
+ ret i32 1
+
+merge_b:
+ ret i32 2
+}
|
| // Check if the default destination only contains an unreachable instruction. | ||
| if (DefaultBB->size() != 1 || | ||
| !isa<UnreachableInst>(DefaultBB->getTerminator())) | ||
| return false; |
There was a problem hiding this comment.
Is the check for size 1 necessary? If the terminator is unreachable we're safe to assume the block really is unreachable.
- in hlsl we can ensure that a branch with an unreachable instruction should is to be treated the whole block being unreachable
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
…lvm#193592) Add a legalization to resolve unreachable default branches in switch statements. These are introduced when the `CorrelatedValuePropagationPass` proves all cases of a switch are covered, leaving the default as an unreachable block. The default destination is replaced with either the common successor block or the first switch case destination. This was discovered from the following case: https://godbolt.org/z/W7zE1GMT6, where we can see that the unrolling of the outer loop is not successful and the switch statement with an `unreachable` branch remains. Assisted by: Claude Opus 4.6
…lvm#193592) Add a legalization to resolve unreachable default branches in switch statements. These are introduced when the `CorrelatedValuePropagationPass` proves all cases of a switch are covered, leaving the default as an unreachable block. The default destination is replaced with either the common successor block or the first switch case destination. This was discovered from the following case: https://godbolt.org/z/W7zE1GMT6, where we can see that the unrolling of the outer loop is not successful and the switch statement with an `unreachable` branch remains. Assisted by: Claude Opus 4.6
…g DirectX" (#194452) The initial landing surfaced 3 somewhat orthogonal issues related to loop unrolling. These are addressed: [here](#193592), [here](#193593) and [here](#193590). These caused these [tests](https://github.com/llvm/llvm-project/actions/runs/24577221310/job/71865579618#step:8:87913) to fail in the offload test suite. We can verify that these are now passing as expected (fixing any of the 3 issues would resolve this and allow us to reland) Some additional tests were added since the revert that are now accounted for and updated in the reland fixes commit. This relands #188792
…en targeting DirectX" (#194452) The initial landing surfaced 3 somewhat orthogonal issues related to loop unrolling. These are addressed: [here](llvm/llvm-project#193592), [here](llvm/llvm-project#193593) and [here](llvm/llvm-project#193590). These caused these [tests](https://github.com/llvm/llvm-project/actions/runs/24577221310/job/71865579618#step:8:87913) to fail in the offload test suite. We can verify that these are now passing as expected (fixing any of the 3 issues would resolve this and allow us to reland) Some additional tests were added since the revert that are now accounted for and updated in the reland fixes commit. This relands llvm/llvm-project#188792
…en targeting DirectX" (#194452) The initial landing surfaced 3 somewhat orthogonal issues related to loop unrolling. These are addressed: [here](llvm/llvm-project#193592), [here](llvm/llvm-project#193593) and [here](llvm/llvm-project#193590). These caused these [tests](https://github.com/llvm/llvm-project/actions/runs/24577221310/job/71865579618#step:8:87913) to fail in the offload test suite. We can verify that these are now passing as expected (fixing any of the 3 issues would resolve this and allow us to reland) Some additional tests were added since the revert that are now accounted for and updated in the reland fixes commit. This relands llvm/llvm-project#188792
Add a legalization to resolve unreachable default branches in switch statements. These are introduced when the
CorrelatedValuePropagationPassproves all cases of a switch are covered, leaving the default as an unreachable block.The default destination is replaced with either the common successor block or the first switch case destination.
This was discovered from the following case: https://godbolt.org/z/W7zE1GMT6, where we can see that the unrolling of the outer loop is not successful and the switch statement with an
unreachablebranch remains.Assisted by: Claude Opus 4.6