[spirv] Fix terminator creation in MergeReturn
When we walk up the control stack to add missing terminators to `if`
instructions, we need to limit this to `if` instructions that were
added by the transform. Otherwise, we may add a terminator to a block
that is still being processed, and end up with multiple terminators.
Fixed: 343597426
Change-Id: If6c345e817a11e68396f5eeed29d38c5b7478323
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/190723
Commit-Queue: James Price <jrprice@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/tint/lang/spirv/writer/raise/merge_return.cc b/src/tint/lang/spirv/writer/raise/merge_return.cc
index 350afa4..077660e0 100644
--- a/src/tint/lang/spirv/writer/raise/merge_return.cc
+++ b/src/tint/lang/spirv/writer/raise/merge_return.cc
@@ -129,6 +129,7 @@
/// @param block the block to process
void ProcessBlock(core::ir::Block* block) {
core::ir::If* inner_if = nullptr;
+ Vector<core::ir::If*, 4> inner_if_stack;
for (auto* inst = *block->begin(); inst;) { // For each instruction in 'block'
// As we're modifying the block that we're iterating over, grab the pointer to the next
// instruction before (potentially) moving 'inst' to another block.
@@ -167,6 +168,7 @@
if (next && (next != fn_return || fn_return->Value()) &&
!tint::IsAnyOf<core::ir::Exit, core::ir::Unreachable>(next)) {
inner_if = CreateIfContinueExecution(ctrl);
+ inner_if_stack.Push(inner_if);
}
}
}
@@ -195,9 +197,10 @@
inner_if->True()->Append(b.ExitIf(inner_if));
}
- // Loop over the 'if' instructions, starting with the inner-most, and add any missing
- // terminating instructions to the blocks holding the 'if'.
- for (auto* i = inner_if; i; i = tint::As<core::ir::If>(i->Block()->Parent())) {
+ // Walk back down the stack of 'if' instructions that were created, and add any missing
+ // terminating instructions to the blocks holding them.
+ while (!inner_if_stack.IsEmpty()) {
+ auto* i = inner_if_stack.Pop();
if (!i->Block()->Terminator() && i->Block()->Parent()) {
// Append the exit instruction to the block holding the 'if'.
Vector<core::ir::InstructionResult*, 8> exit_args = i->Results();
diff --git a/src/tint/lang/spirv/writer/raise/merge_return_test.cc b/src/tint/lang/spirv/writer/raise/merge_return_test.cc
index 9cb2f0a..7b7c0f1 100644
--- a/src/tint/lang/spirv/writer/raise/merge_return_test.cc
+++ b/src/tint/lang/spirv/writer/raise/merge_return_test.cc
@@ -1413,6 +1413,306 @@
EXPECT_EQ(expect, str());
}
+TEST_F(SpirvWriter_MergeReturnTest, IfElse_NestedConsecutives) {
+ auto* value = b.FunctionParam(ty.i32());
+ auto* func = b.Function("foo", ty.i32());
+ func->SetParams({value});
+
+ b.Append(func->Block(), [&] {
+ auto* outer = b.If(b.Equal(ty.bool_(), value, 1_i));
+ b.Append(outer->True(), [&] {
+ auto* middle_first = b.If(b.Equal(ty.bool_(), value, 2_i));
+ b.Append(middle_first->True(), [&] { //
+ b.Return(func, 202_i);
+ });
+
+ auto* middle_second = b.If(b.Equal(ty.bool_(), value, 3_i));
+ b.Append(middle_second->True(), [&] {
+ auto* inner_first = b.If(b.Equal(ty.bool_(), value, 4_i));
+ b.Append(inner_first->True(), [&] { //
+ b.Return(func, 404_i);
+ });
+
+ auto* inner_second = b.If(b.Equal(ty.bool_(), value, 5_i));
+ b.Append(inner_second->True(), [&] { //
+ b.Return(func, 505_i);
+ });
+
+ b.ExitIf(middle_second);
+ });
+
+ b.ExitIf(outer);
+ });
+
+ b.Return(func, 606_i);
+ });
+
+ auto* src = R"(
+%foo = func(%2:i32):i32 {
+ $B1: {
+ %3:bool = eq %2, 1i
+ if %3 [t: $B2] { # if_1
+ $B2: { # true
+ %4:bool = eq %2, 2i
+ if %4 [t: $B3] { # if_2
+ $B3: { # true
+ ret 202i
+ }
+ }
+ %5:bool = eq %2, 3i
+ if %5 [t: $B4] { # if_3
+ $B4: { # true
+ %6:bool = eq %2, 4i
+ if %6 [t: $B5] { # if_4
+ $B5: { # true
+ ret 404i
+ }
+ }
+ %7:bool = eq %2, 5i
+ if %7 [t: $B6] { # if_5
+ $B6: { # true
+ ret 505i
+ }
+ }
+ exit_if # if_3
+ }
+ }
+ exit_if # if_1
+ }
+ }
+ ret 606i
+ }
+}
+)";
+ EXPECT_EQ(src, str());
+
+ auto* expect = R"(
+%foo = func(%2:i32):i32 {
+ $B1: {
+ %return_value:ptr<function, i32, read_write> = var
+ %continue_execution:ptr<function, bool, read_write> = var, true
+ %5:bool = eq %2, 1i
+ if %5 [t: $B2] { # if_1
+ $B2: { # true
+ %6:bool = eq %2, 2i
+ if %6 [t: $B3] { # if_2
+ $B3: { # true
+ store %continue_execution, false
+ store %return_value, 202i
+ exit_if # if_2
+ }
+ }
+ %7:bool = load %continue_execution
+ if %7 [t: $B4] { # if_3
+ $B4: { # true
+ %8:bool = eq %2, 3i
+ if %8 [t: $B5] { # if_4
+ $B5: { # true
+ %9:bool = eq %2, 4i
+ if %9 [t: $B6] { # if_5
+ $B6: { # true
+ store %continue_execution, false
+ store %return_value, 404i
+ exit_if # if_5
+ }
+ }
+ %10:bool = load %continue_execution
+ if %10 [t: $B7] { # if_6
+ $B7: { # true
+ %11:bool = eq %2, 5i
+ if %11 [t: $B8] { # if_7
+ $B8: { # true
+ store %continue_execution, false
+ store %return_value, 505i
+ exit_if # if_7
+ }
+ }
+ exit_if # if_6
+ }
+ }
+ exit_if # if_4
+ }
+ }
+ exit_if # if_3
+ }
+ }
+ exit_if # if_1
+ }
+ }
+ %12:bool = load %continue_execution
+ if %12 [t: $B9] { # if_8
+ $B9: { # true
+ store %return_value, 606i
+ exit_if # if_8
+ }
+ }
+ %13:i32 = load %return_value
+ ret %13
+ }
+}
+)";
+
+ Run(MergeReturn);
+
+ EXPECT_EQ(expect, str());
+}
+
+TEST_F(SpirvWriter_MergeReturnTest, IfElse_NestedConsecutives_WithResults) {
+ auto* value = b.FunctionParam(ty.i32());
+ auto* func = b.Function("foo", ty.i32());
+ func->SetParams({value});
+
+ b.Append(func->Block(), [&] {
+ auto* outer_result = b.InstructionResult(ty.i32());
+ auto* outer = b.If(b.Equal(ty.bool_(), value, 1_i));
+ outer->SetResults(Vector{outer_result});
+ b.Append(outer->True(), [&] {
+ auto* middle_first = b.If(b.Equal(ty.bool_(), value, 2_i));
+ b.Append(middle_first->True(), [&] { //
+ b.Return(func, 202_i);
+ });
+
+ auto middle_result = b.InstructionResult(ty.i32());
+ auto* middle_second = b.If(b.Equal(ty.bool_(), value, 3_i));
+ middle_second->SetResults(Vector{middle_result});
+ b.Append(middle_second->True(), [&] {
+ auto* inner_first = b.If(b.Equal(ty.bool_(), value, 4_i));
+ b.Append(inner_first->True(), [&] { //
+ b.Return(func, 404_i);
+ });
+
+ auto inner_result = b.InstructionResult(ty.i32());
+ auto* inner_second = b.If(b.Equal(ty.bool_(), value, 5_i));
+ inner_second->SetResults(Vector{inner_result});
+ b.Append(inner_second->True(), [&] { //
+ b.ExitIf(inner_second, 505_i);
+ });
+
+ b.ExitIf(middle_second, inner_result);
+ });
+
+ b.ExitIf(outer, middle_result);
+ });
+
+ b.Return(func, outer_result);
+ });
+
+ auto* src = R"(
+%foo = func(%2:i32):i32 {
+ $B1: {
+ %3:bool = eq %2, 1i
+ %4:i32 = if %3 [t: $B2] { # if_1
+ $B2: { # true
+ %5:bool = eq %2, 2i
+ if %5 [t: $B3] { # if_2
+ $B3: { # true
+ ret 202i
+ }
+ }
+ %6:bool = eq %2, 3i
+ %7:i32 = if %6 [t: $B4] { # if_3
+ $B4: { # true
+ %8:bool = eq %2, 4i
+ if %8 [t: $B5] { # if_4
+ $B5: { # true
+ ret 404i
+ }
+ }
+ %9:bool = eq %2, 5i
+ %10:i32 = if %9 [t: $B6] { # if_5
+ $B6: { # true
+ exit_if 505i # if_5
+ }
+ # implicit false block: exit_if undef
+ }
+ exit_if %10 # if_3
+ }
+ # implicit false block: exit_if undef
+ }
+ exit_if %7 # if_1
+ }
+ # implicit false block: exit_if undef
+ }
+ ret %4
+ }
+}
+)";
+ EXPECT_EQ(src, str());
+
+ auto* expect = R"(
+%foo = func(%2:i32):i32 {
+ $B1: {
+ %return_value:ptr<function, i32, read_write> = var
+ %continue_execution:ptr<function, bool, read_write> = var, true
+ %5:bool = eq %2, 1i
+ %6:i32 = if %5 [t: $B2] { # if_1
+ $B2: { # true
+ %7:bool = eq %2, 2i
+ if %7 [t: $B3] { # if_2
+ $B3: { # true
+ store %continue_execution, false
+ store %return_value, 202i
+ exit_if # if_2
+ }
+ }
+ %8:bool = load %continue_execution
+ %9:i32 = if %8 [t: $B4] { # if_3
+ $B4: { # true
+ %10:bool = eq %2, 3i
+ %11:i32 = if %10 [t: $B5] { # if_4
+ $B5: { # true
+ %12:bool = eq %2, 4i
+ if %12 [t: $B6] { # if_5
+ $B6: { # true
+ store %continue_execution, false
+ store %return_value, 404i
+ exit_if # if_5
+ }
+ }
+ %13:bool = load %continue_execution
+ %14:i32 = if %13 [t: $B7] { # if_6
+ $B7: { # true
+ %15:bool = eq %2, 5i
+ %16:i32 = if %15 [t: $B8] { # if_7
+ $B8: { # true
+ exit_if 505i # if_7
+ }
+ # implicit false block: exit_if undef
+ }
+ exit_if %16 # if_6
+ }
+ # implicit false block: exit_if undef
+ }
+ exit_if %14 # if_4
+ }
+ # implicit false block: exit_if undef
+ }
+ exit_if %11 # if_3
+ }
+ # implicit false block: exit_if undef
+ }
+ exit_if %9 # if_1
+ }
+ # implicit false block: exit_if undef
+ }
+ %17:bool = load %continue_execution
+ if %17 [t: $B9] { # if_8
+ $B9: { # true
+ store %return_value, %6
+ exit_if # if_8
+ }
+ }
+ %18:i32 = load %return_value
+ ret %18
+ }
+}
+)";
+
+ Run(MergeReturn);
+
+ EXPECT_EQ(expect, str());
+}
+
TEST_F(SpirvWriter_MergeReturnTest, Loop_UnconditionalReturnInBody) {
auto* func = b.Function("foo", ty.i32());
diff --git a/test/tint/bug/chromium/343597426.wgsl b/test/tint/bug/chromium/343597426.wgsl
new file mode 100644
index 0000000..5f34d79
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl
@@ -0,0 +1,14 @@
+fn foo(a: bool, b: bool, c: bool, d: bool, e: bool) {
+ if a {
+ if b {
+ return;
+ }
+ if c {
+ if d {
+ return;
+ }
+ if e {
+ }
+ }
+ }
+}
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.dxc.hlsl b/test/tint/bug/chromium/343597426.wgsl.expected.dxc.hlsl
new file mode 100644
index 0000000..b06623c
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.dxc.hlsl
@@ -0,0 +1,19 @@
+[numthreads(1, 1, 1)]
+void unused_entry_point() {
+ return;
+}
+
+void foo(bool a, bool b, bool c, bool d, bool e) {
+ if (a) {
+ if (b) {
+ return;
+ }
+ if (c) {
+ if (d) {
+ return;
+ }
+ if (e) {
+ }
+ }
+ }
+}
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.fxc.hlsl b/test/tint/bug/chromium/343597426.wgsl.expected.fxc.hlsl
new file mode 100644
index 0000000..b06623c
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.fxc.hlsl
@@ -0,0 +1,19 @@
+[numthreads(1, 1, 1)]
+void unused_entry_point() {
+ return;
+}
+
+void foo(bool a, bool b, bool c, bool d, bool e) {
+ if (a) {
+ if (b) {
+ return;
+ }
+ if (c) {
+ if (d) {
+ return;
+ }
+ if (e) {
+ }
+ }
+ }
+}
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.glsl b/test/tint/bug/chromium/343597426.wgsl.expected.glsl
new file mode 100644
index 0000000..82976e7
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.glsl
@@ -0,0 +1,21 @@
+#version 310 es
+
+layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;
+void unused_entry_point() {
+ return;
+}
+void foo(bool a, bool b, bool c, bool d, bool e) {
+ if (a) {
+ if (b) {
+ return;
+ }
+ if (c) {
+ if (d) {
+ return;
+ }
+ if (e) {
+ }
+ }
+ }
+}
+
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.ir.msl b/test/tint/bug/chromium/343597426.wgsl.expected.ir.msl
new file mode 100644
index 0000000..8f6ddbb
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.ir.msl
@@ -0,0 +1,17 @@
+#include <metal_stdlib>
+using namespace metal;
+
+void foo(bool a, bool b, bool c, bool d, bool e) {
+ if (a) {
+ if (b) {
+ return;
+ }
+ if (c) {
+ if (d) {
+ return;
+ }
+ if (e) {
+ }
+ }
+ }
+}
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.ir.spvasm b/test/tint/bug/chromium/343597426.wgsl.expected.ir.spvasm
new file mode 100644
index 0000000..cc1c1a7
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.ir.spvasm
@@ -0,0 +1,76 @@
+; SPIR-V
+; Version: 1.3
+; Generator: Google Tint Compiler; 1
+; Bound: 33
+; Schema: 0
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint GLCompute %unused_entry_point "unused_entry_point"
+ OpExecutionMode %unused_entry_point LocalSize 1 1 1
+ OpName %foo "foo"
+ OpName %a "a"
+ OpName %b "b"
+ OpName %c "c"
+ OpName %d "d"
+ OpName %e "e"
+ OpName %continue_execution "continue_execution"
+ OpName %unused_entry_point "unused_entry_point"
+ %void = OpTypeVoid
+ %bool = OpTypeBool
+ %9 = OpTypeFunction %void %bool %bool %bool %bool %bool
+%_ptr_Function_bool = OpTypePointer Function %bool
+ %true = OpConstantTrue %bool
+ %false = OpConstantFalse %bool
+ %31 = OpTypeFunction %void
+ %foo = OpFunction %void None %9
+ %a = OpFunctionParameter %bool
+ %b = OpFunctionParameter %bool
+ %c = OpFunctionParameter %bool
+ %d = OpFunctionParameter %bool
+ %e = OpFunctionParameter %bool
+ %10 = OpLabel
+%continue_execution = OpVariable %_ptr_Function_bool Function
+ OpStore %continue_execution %true
+ OpSelectionMerge %14 None
+ OpBranchConditional %a %15 %14
+ %15 = OpLabel
+ OpSelectionMerge %16 None
+ OpBranchConditional %b %17 %16
+ %17 = OpLabel
+ OpStore %continue_execution %false
+ OpBranch %16
+ %16 = OpLabel
+ %19 = OpLoad %bool %continue_execution
+ OpSelectionMerge %20 None
+ OpBranchConditional %19 %21 %20
+ %21 = OpLabel
+ OpSelectionMerge %22 None
+ OpBranchConditional %c %23 %22
+ %23 = OpLabel
+ OpSelectionMerge %24 None
+ OpBranchConditional %d %25 %24
+ %25 = OpLabel
+ OpStore %continue_execution %false
+ OpBranch %24
+ %24 = OpLabel
+ %26 = OpLoad %bool %continue_execution
+ OpSelectionMerge %27 None
+ OpBranchConditional %26 %28 %27
+ %28 = OpLabel
+ OpSelectionMerge %29 None
+ OpBranchConditional %e %29 %29
+ %29 = OpLabel
+ OpBranch %27
+ %27 = OpLabel
+ OpBranch %22
+ %22 = OpLabel
+ OpBranch %20
+ %20 = OpLabel
+ OpBranch %14
+ %14 = OpLabel
+ OpReturn
+ OpFunctionEnd
+%unused_entry_point = OpFunction %void None %31
+ %32 = OpLabel
+ OpReturn
+ OpFunctionEnd
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.msl b/test/tint/bug/chromium/343597426.wgsl.expected.msl
new file mode 100644
index 0000000..b9dd7c8
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.msl
@@ -0,0 +1,18 @@
+#include <metal_stdlib>
+
+using namespace metal;
+void foo(bool a, bool b, bool c, bool d, bool e) {
+ if (a) {
+ if (b) {
+ return;
+ }
+ if (c) {
+ if (d) {
+ return;
+ }
+ if (e) {
+ }
+ }
+ }
+}
+
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.spvasm b/test/tint/bug/chromium/343597426.wgsl.expected.spvasm
new file mode 100644
index 0000000..1673b20
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.spvasm
@@ -0,0 +1,79 @@
+; SPIR-V
+; Version: 1.3
+; Generator: Google Tint Compiler; 0
+; Bound: 36
+; Schema: 0
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint GLCompute %unused_entry_point "unused_entry_point"
+ OpExecutionMode %unused_entry_point LocalSize 1 1 1
+ OpName %unused_entry_point "unused_entry_point"
+ OpName %foo "foo"
+ OpName %a "a"
+ OpName %b "b"
+ OpName %c "c"
+ OpName %d "d"
+ OpName %e "e"
+ OpName %tint_return_flag "tint_return_flag"
+ %void = OpTypeVoid
+ %1 = OpTypeFunction %void
+ %bool = OpTypeBool
+ %5 = OpTypeFunction %void %bool %bool %bool %bool %bool
+%_ptr_Function_bool = OpTypePointer Function %bool
+ %16 = OpConstantNull %bool
+ %true = OpConstantTrue %bool
+%unused_entry_point = OpFunction %void None %1
+ %4 = OpLabel
+ OpReturn
+ OpFunctionEnd
+ %foo = OpFunction %void None %5
+ %a = OpFunctionParameter %bool
+ %b = OpFunctionParameter %bool
+ %c = OpFunctionParameter %bool
+ %d = OpFunctionParameter %bool
+ %e = OpFunctionParameter %bool
+ %13 = OpLabel
+%tint_return_flag = OpVariable %_ptr_Function_bool Function %16
+ OpSelectionMerge %17 None
+ OpBranchConditional %a %18 %17
+ %18 = OpLabel
+ OpSelectionMerge %19 None
+ OpBranchConditional %b %20 %19
+ %20 = OpLabel
+ OpStore %tint_return_flag %true
+ OpBranch %19
+ %19 = OpLabel
+ %23 = OpLoad %bool %tint_return_flag
+ %22 = OpLogicalNot %bool %23
+ OpSelectionMerge %24 None
+ OpBranchConditional %22 %25 %24
+ %25 = OpLabel
+ OpSelectionMerge %26 None
+ OpBranchConditional %c %27 %26
+ %27 = OpLabel
+ OpSelectionMerge %28 None
+ OpBranchConditional %d %29 %28
+ %29 = OpLabel
+ OpStore %tint_return_flag %true
+ OpBranch %28
+ %28 = OpLabel
+ %31 = OpLoad %bool %tint_return_flag
+ %30 = OpLogicalNot %bool %31
+ OpSelectionMerge %32 None
+ OpBranchConditional %30 %33 %32
+ %33 = OpLabel
+ OpSelectionMerge %34 None
+ OpBranchConditional %e %35 %34
+ %35 = OpLabel
+ OpBranch %34
+ %34 = OpLabel
+ OpBranch %32
+ %32 = OpLabel
+ OpBranch %26
+ %26 = OpLabel
+ OpBranch %24
+ %24 = OpLabel
+ OpBranch %17
+ %17 = OpLabel
+ OpReturn
+ OpFunctionEnd
diff --git a/test/tint/bug/chromium/343597426.wgsl.expected.wgsl b/test/tint/bug/chromium/343597426.wgsl.expected.wgsl
new file mode 100644
index 0000000..0310e83
--- /dev/null
+++ b/test/tint/bug/chromium/343597426.wgsl.expected.wgsl
@@ -0,0 +1,14 @@
+fn foo(a : bool, b : bool, c : bool, d : bool, e : bool) {
+ if (a) {
+ if (b) {
+ return;
+ }
+ if (c) {
+ if (d) {
+ return;
+ }
+ if (e) {
+ }
+ }
+ }
+}
