[spirv-reader] Uses in phis count as uses
This is required so we actually emit values that are only used
in phis.
Bug: tint:3, tint:215
Change-Id: I9d957a697839b8d09246905c3f28064f0bc01731
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27701
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 724ff47..f4ae488 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -3244,27 +3244,25 @@
const auto* block_info = GetBlockInfo(block_id);
const auto block_pos = block_info->pos;
for (const auto& inst : *(block_info->basic_block)) {
- // Update the usage span for IDs used by this instruction.
- // But skip uses in OpPhi because they are handled differently.
- if (inst.opcode() != SpvOpPhi) {
- inst.ForEachInId([this, block_pos, block_info](const uint32_t* id_ptr) {
- auto* def_info = GetDefInfo(*id_ptr);
- if (def_info) {
- def_info->num_uses++;
- def_info->last_use_pos =
- std::max(def_info->last_use_pos, block_pos);
+ // Update bookkeeping for locally-defined IDs used by this instruction.
+ inst.ForEachInId([this, block_pos, block_info](const uint32_t* id_ptr) {
+ auto* def_info = GetDefInfo(*id_ptr);
+ if (def_info) {
+ // Update usage count.
+ def_info->num_uses++;
+ // Update usage span.
+ def_info->last_use_pos = std::max(def_info->last_use_pos, block_pos);
- // Determine whether this ID is defined in a different construct
- // from this use.
- const auto defining_block = block_order_[def_info->block_pos];
- const auto* def_in_construct =
- GetBlockInfo(defining_block)->construct;
- if (def_in_construct != block_info->construct) {
- def_info->used_in_another_construct = true;
- }
+ // Determine whether this ID is defined in a different construct
+ // from this use.
+ const auto defining_block = block_order_[def_info->block_pos];
+ const auto* def_in_construct =
+ GetBlockInfo(defining_block)->construct;
+ if (def_in_construct != block_info->construct) {
+ def_info->used_in_another_construct = true;
}
- });
- }
+ }
+ });
if (inst.opcode() == SpvOpPhi) {
// Declare a name for the variable used to carry values to a phi.
diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc
index ebbac3d..d4a4601 100644
--- a/src/reader/spirv/function_var_test.cc
+++ b/src/reader/spirv/function_var_test.cc
@@ -1363,23 +1363,13 @@
}
}
}
- VariableDeclStatement{
- Variable{
- x_4
- none
- __u32
- {
- Binary{
- Identifier{x_2}
- add
- ScalarConstructor{1}
- }
- }
- }
- }
Assignment{
Identifier{x_2_phi}
- Identifier{x_4}
+ Binary{
+ Identifier{x_2}
+ add
+ ScalarConstructor{1}
+ }
}
Assignment{
Identifier{x_3_phi}
@@ -1497,6 +1487,13 @@
Loop{
VariableDeclStatement{
Variable{
+ x_4
+ function
+ __u32
+ }
+ }
+ VariableDeclStatement{
+ Variable{
x_2
none
__u32
@@ -1524,18 +1521,12 @@
}
}
continuing {
- VariableDeclStatement{
- Variable{
- x_4
- none
- __u32
- {
- Binary{
- Identifier{x_2}
- add
- ScalarConstructor{1}
- }
- }
+ Assignment{
+ Identifier{x_4}
+ Binary{
+ Identifier{x_2}
+ add
+ ScalarConstructor{1}
}
}
Assignment{
@@ -1634,6 +1625,13 @@
Loop{
VariableDeclStatement{
Variable{
+ x_7
+ function
+ __u32
+ }
+ }
+ VariableDeclStatement{
+ Variable{
x_2
none
__u32
@@ -1689,18 +1687,12 @@
}
}
continuing {
- VariableDeclStatement{
- Variable{
- x_7
- none
- __u32
- {
- Binary{
- Identifier{x_4}
- add
- Identifier{x_6}
- }
- }
+ Assignment{
+ Identifier{x_7}
+ Binary{
+ Identifier{x_4}
+ add
+ Identifier{x_6}
}
}
Assignment{
@@ -1957,6 +1949,103 @@
)")) << ToString(fe.ast_body());
}
+TEST_F(SpvParserTest, EmitStatement_UseInPhiCountsAsUse) {
+ // From crbug.com/215
+ // If the only use of a combinatorially computed ID is as the value
+ // in an OpPhi, then we still have to emit it. The algorithm fix
+ // is to always count uses in Phis.
+ // This is the reduced case from the bug report.
+ //
+ // The only use of %12 is in the phi.
+ // The only use of %11 is in %12.
+ // Both definintions need to be emitted to the output.
+ auto assembly = Preamble() + R"(
+ %100 = OpFunction %void None %voidfn
+
+ %10 = OpLabel
+ %11 = OpLogicalAnd %bool %true %true
+ %12 = OpLogicalNot %bool %11 ;
+ OpSelectionMerge %99 None
+ OpBranchConditional %true %20 %99
+
+ %20 = OpLabel
+ OpBranch %99
+
+ %99 = OpLabel
+ %101 = OpPhi %bool %11 %10 %12 %20
+ OpReturn
+
+ OpFunctionEnd
+
+ )";
+ auto* p = parser(test::Assemble(assembly));
+ ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
+ FunctionEmitter fe(p, *spirv_function(100));
+ EXPECT_TRUE(fe.EmitBody()) << p->error();
+
+ EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(VariableDeclStatement{
+ Variable{
+ x_101_phi
+ function
+ __bool
+ }
+}
+VariableDeclStatement{
+ Variable{
+ x_11
+ none
+ __bool
+ {
+ Binary{
+ ScalarConstructor{true}
+ logical_and
+ ScalarConstructor{true}
+ }
+ }
+ }
+}
+VariableDeclStatement{
+ Variable{
+ x_12
+ none
+ __bool
+ {
+ UnaryOp{
+ not
+ Identifier{x_11}
+ }
+ }
+ }
+}
+Assignment{
+ Identifier{x_101_phi}
+ Identifier{x_11}
+}
+If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ Assignment{
+ Identifier{x_101_phi}
+ Identifier{x_12}
+ }
+ }
+}
+VariableDeclStatement{
+ Variable{
+ x_101
+ none
+ __bool
+ {
+ Identifier{x_101_phi}
+ }
+ }
+}
+Return{}
+)")) << ToString(fe.ast_body());
+}
+
} // namespace
} // namespace spirv
} // namespace reader
