spirv-reader: fix position calculation of for phi var decl
Fixed: tint:495
Change-Id: I93f1b289eb33c2bda8b661f591a1c8dde775b1ab
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46220
Commit-Queue: David Neto <dneto@google.com>
Commit-Queue: Alan Baker <alanbaker@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Alan Baker <alanbaker@google.com>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 5139f9f..16012fe 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -3961,7 +3961,7 @@
// of the predecessor block.
pred_block_info->phi_assignments.push_back({phi_id, value_id});
first_pos = std::min(first_pos, pred_block_info->pos);
- last_pos = std::min(last_pos, pred_block_info->pos);
+ last_pos = std::max(last_pos, pred_block_info->pos);
}
}
diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc
index 790586c..1543518 100644
--- a/src/reader/spirv/function_var_test.cc
+++ b/src/reader/spirv/function_var_test.cc
@@ -1811,6 +1811,13 @@
}
}
Loop{
+ VariableDeclStatement{
+ Variable{
+ x_2_phi
+ function
+ __u32
+ }
+ }
If{
(
Identifier[not set]{x_101}
@@ -1819,13 +1826,6 @@
Break{}
}
}
- VariableDeclStatement{
- Variable{
- x_2_phi
- function
- __u32
- }
- }
If{
(
Identifier[not set]{x_102}
@@ -1865,7 +1865,7 @@
}
Return{}
)";
- EXPECT_EQ(expect, got);
+ EXPECT_EQ(expect, got) << got;
}
TEST_F(SpvParserTest, EmitStatement_Phi_FromHeaderAndThen) {
@@ -1933,6 +1933,13 @@
}
}
Loop{
+ VariableDeclStatement{
+ Variable{
+ x_2_phi
+ function
+ __u32
+ }
+ }
If{
(
Identifier[not set]{x_101}
@@ -1941,13 +1948,6 @@
Break{}
}
}
- VariableDeclStatement{
- Variable{
- x_2_phi
- function
- __u32
- }
- }
Assignment{
Identifier[not set]{x_2_phi}
ScalarConstructor[not set]{0}
@@ -1982,7 +1982,108 @@
}
Return{}
)";
- EXPECT_EQ(expect, got);
+ EXPECT_EQ(expect, got) << got;
+}
+
+TEST_F(SpvParserTest,
+ EmitStatement_Phi_InMerge_PredecessorsDominatdByNestedSwitchCase) {
+ // This is the essence of the bug report from crbug.com/tint/495
+ auto assembly = Preamble() + R"(
+ %cond = OpConstantTrue %bool
+ %pty = OpTypePointer Private %uint
+ %1 = OpVariable %pty Private
+ %boolpty = OpTypePointer Private %bool
+ %7 = OpVariable %boolpty Private
+ %8 = OpVariable %boolpty Private
+
+ %100 = OpFunction %void None %voidfn
+
+ %10 = OpLabel
+ OpSelectionMerge %99 None
+ OpSwitch %uint_1 %20 0 %20 1 %30
+
+ %20 = OpLabel ; case 0
+ OpBranch %30 ;; fall through
+
+ %30 = OpLabel ; case 1
+ OpSelectionMerge %50 None
+ OpBranchConditional %true %40 %45
+
+ %40 = OpLabel
+ OpBranch %50
+
+ %45 = OpLabel
+ OpBranch %99 ; break
+
+ %50 = OpLabel ; end the case
+ OpBranch %99
+
+ %99 = OpLabel
+ ; predecessors are all dominated by case construct head at %30
+ %phi = OpPhi %uint %uint_0 %45 %uint_1 %50
+ OpReturn
+
+ OpFunctionEnd
+ )";
+ auto p = parser(test::Assemble(assembly));
+ ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
+ FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
+ EXPECT_TRUE(fe.EmitBody()) << p->error();
+
+ auto got = ToString(p->builder(), fe.ast_body());
+ auto* expect = R"(VariableDeclStatement{
+ Variable{
+ x_35_phi
+ function
+ __u32
+ }
+}
+Switch{
+ ScalarConstructor[not set]{1}
+ {
+ Default{
+ Fallthrough{}
+ }
+ Case 0{
+ Fallthrough{}
+ }
+ Case 1{
+ If{
+ (
+ ScalarConstructor[not set]{true}
+ )
+ {
+ }
+ }
+ Else{
+ {
+ Assignment{
+ Identifier[not set]{x_35_phi}
+ ScalarConstructor[not set]{0}
+ }
+ Break{}
+ }
+ }
+ Assignment{
+ Identifier[not set]{x_35_phi}
+ ScalarConstructor[not set]{1}
+ }
+ }
+ }
+}
+VariableDeclStatement{
+ VariableConst{
+ x_35
+ none
+ __u32
+ {
+ Identifier[not set]{x_35_phi}
+ }
+ }
+}
+Return{}
+)";
+ EXPECT_EQ(expect, got) << got << assembly;
}
TEST_F(SpvParserTest, EmitStatement_UseInPhiCountsAsUse) {
