From ae5437cc87fcc0c7adc148e5cb532e38d853cf36 Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 1 Jun 2021 18:52:01 +0000 Subject: [PATCH] spirv-reader: ignore phi inputs coming from non-ordered blocks Bug: tint:804 Change-Id: I789c05a31d869052036351b8c173ef8cd7191cc2 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/52841 Auto-Submit: David Neto Commit-Queue: James Price Kokoro: Kokoro Reviewed-by: James Price --- src/reader/spirv/function.cc | 2 +- src/reader/spirv/function.h | 7 +++ src/reader/spirv/function_var_test.cc | 78 +++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index b55ecd7a76..61dae65ac4 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -4362,7 +4362,7 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { auto* pred_block_info = GetBlockInfo(pred_block_id); // The predecessor might not be in the block order at all, so we // need this guard. - if (pred_block_info) { + if (IsInBlockOrder(pred_block_info)) { // Record the assignment that needs to occur at the end // of the predecessor block. pred_block_info->phi_assignments.push_back({phi_id, value_id}); diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index f0a2c975fd..2bfe745680 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -766,6 +766,13 @@ class FunctionEmitter { return where->second.get(); } + /// Is the block, represented by info, in the structured block order? + /// @param info the block + /// @returns true if the block is in the structured block order. + bool IsInBlockOrder(const BlockInfo* info) const { + return info && info->pos != kInvalidBlockPos; + } + /// Gets the local definition info for a result ID. /// @param id the SPIR-V ID of local definition. /// @returns the definition info for the given ID, if it exists, or nullptr diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc index 8bacfad9fd..dc474a0cec 100644 --- a/src/reader/spirv/function_var_test.cc +++ b/src/reader/spirv/function_var_test.cc @@ -2234,6 +2234,84 @@ Return{} EXPECT_EQ(expect, got); } +TEST_F(SpvParserFunctionVarTest, + EmitStatement_Phi_ValueFromBlockNotInBlockOrderIgnored) { + // From crbug.com/tint/804 + const auto assembly = Preamble() + R"( + %float_42 = OpConstant %float 42.0 + %cond = OpUndef %bool + + %100 = OpFunction %void None %voidfn + %10 = OpLabel + OpBranch %30 + + ; unreachable + %20 = OpLabel + %499 = OpFAdd %float %float_42 %float_42 + %500 = OpFAdd %float %499 %float_42 + OpBranch %25 + + %25 = OpLabel + OpBranch %80 + + + %30 = OpLabel + OpLoopMerge %90 %80 None + OpBranchConditional %cond %90 %40 + + %40 = OpLabel + OpBranch %90 + + %80 = OpLabel ; unreachable continue target + ; but "dominated" by %20 and %25 + %81 = OpPhi %float %500 %25 + OpBranch %30 ; backedge + + %90 = OpLabel + OpReturn + OpFunctionEnd +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly; + auto fe = p->function_emitter(100); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + + const auto* expected = R"(Loop{ + If{ + ( + ScalarConstructor[not set]{false} + ) + { + Break{} + } + } + Break{} + continuing { + VariableDeclStatement{ + Variable{ + x_81_phi_1 + none + __f32 + } + } + VariableDeclStatement{ + VariableConst{ + x_81 + none + __f32 + { + Identifier[not set]{x_81_phi_1} + } + } + } + } +} +Return{} +)"; + const auto got = ToString(p->builder(), fe.ast_body()); + EXPECT_EQ(got, expected); +} + } // namespace } // namespace spirv } // namespace reader