From 39e83353f03a9d9424c998104339ba1c2140fec4 Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 9 Jun 2020 20:07:48 +0000 Subject: [PATCH] [spirv-reader] Avoid emitting empty elses Produce less noise in ASTs for a common case. Also test that an empty continuing construct doesn't show up in the AST. That's currently handled by the AST code. We want to keep this behaviour even if the AST implementation changes. Right now code change is needed when emitting the start of a continuing construct. Bug: tint:3 Change-Id: I96a12087e305c64647561f65d87acda907ae9c42 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/22844 Reviewed-by: dan sinclair --- src/reader/spirv/function.cc | 15 ++- src/reader/spirv/function_cfg_test.cc | 152 ++++++++++++++------------ 2 files changed, 89 insertions(+), 78 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 0cd954e693..6203a5d388 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -1790,12 +1790,15 @@ bool FunctionEmitter::EmitIfStart(const BlockInfo& block_info) { auto push_else = [this, if_stmt, else_end, construct]() { // Push the else clause onto the stack first. PushNewStatementBlock(construct, else_end, [if_stmt](StatementBlock* s) { - // The "else" consists of the statement list from the top of statments - // stack, without an elseif condition. - ast::ElseStatementList else_stmts; - else_stmts.emplace_back(std::make_unique( - nullptr, std::move(s->statements))); - if_stmt->set_else_statements(std::move(else_stmts)); + // Only set the else-clause if there are statements to fill it. + if (!s->statements.empty()) { + // The "else" consists of the statement list from the top of statments + // stack, without an elseif condition. + ast::ElseStatementList else_stmts; + else_stmts.emplace_back(std::make_unique( + nullptr, std::move(s->statements))); + if_stmt->set_else_statements(std::move(else_stmts)); + } }); }; diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc index 52b42f1dcd..4150caecbb 100644 --- a/src/reader/spirv/function_cfg_test.cc +++ b/src/reader/spirv/function_cfg_test.cc @@ -7406,10 +7406,6 @@ TEST_F(SpvParserTest, EmitBody_If_Empty) { { } } -Else{ - { - } -} Return{} )")); } @@ -7452,10 +7448,6 @@ If{ } } } -Else{ - { - } -} Assignment{ Identifier{var} ScalarConstructor{999} @@ -7684,10 +7676,6 @@ If{ } } } -Else{ - { - } -} Assignment{ Identifier{var} ScalarConstructor{3} @@ -7828,10 +7816,6 @@ If{ } } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{3} @@ -8252,10 +8236,6 @@ Loop{ } } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{5} @@ -8504,10 +8484,6 @@ TEST_F(SpvParserTest, EmitBody_Loop_NestedIfContinue) { Continue{} } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{2} @@ -8849,10 +8825,6 @@ TEST_F(SpvParserTest, EmitBody_Return_InsideIf) { Return{} } } -Else{ - { - } -} Return{} )")) << ToString(fe.ast_body()); } @@ -8960,10 +8932,6 @@ TEST_F(SpvParserTest, EmitBody_ReturnValue_InsideIf) { } } } -Else{ - { - } -} Return{ { ScalarConstructor{3} @@ -9067,10 +9035,6 @@ TEST_F(SpvParserTest, EmitBody_Kill_InsideIf) { Kill{} } } -Else{ - { - } -} Kill{} )")) << ToString(fe.ast_body()); } @@ -9153,10 +9117,6 @@ TEST_F(SpvParserTest, EmitBody_Unreachable_InsideIf) { Return{} } } -Else{ - { - } -} Return{} )")) << ToString(fe.ast_body()); } @@ -9507,10 +9467,6 @@ TEST_F(SpvParserTest, EmitBody_Branch_LoopContinue_BeforeLast) { Continue{} } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{2} @@ -9559,10 +9515,6 @@ TEST_F(SpvParserTest, EmitBody_Branch_IfBreak_FromThen) { } } } -Else{ - { - } -} Assignment{ Identifier{var} ScalarConstructor{2} @@ -10188,10 +10140,6 @@ Loop{ } } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{3} @@ -10286,10 +10234,6 @@ Loop{ } } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{3} @@ -10648,10 +10592,6 @@ Loop{ Continue{} } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{4} @@ -10671,6 +10611,86 @@ Return{} )")) << ToString(fe.ast_body()); } +TEST_F( + SpvParserTest, + EmitBody_BranchConditional_Continue_Continue_AfterHeader_Conditional_EmptyContinuing) { + // Like the previous tests, but with an empty continuing clause. + auto* p = parser(test::Assemble(CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpStore %var %uint_0 + OpBranch %20 + + %20 = OpLabel + OpStore %var %uint_1 + OpLoopMerge %99 %80 None + OpBranch %30 + + %30 = OpLabel + OpStore %var %uint_2 + OpSelectionMerge %50 None + OpBranchConditional %cond2 %40 %50 + + %40 = OpLabel + OpStore %var %uint_3 + OpBranchConditional %cond3 %80 %80 ; to continue + + %50 = OpLabel ; merge for selection + OpStore %var %uint_4 + OpBranch %80 + + %80 = OpLabel ; continue target + ; no statements here. + OpBranch %20 + + %99 = OpLabel + OpStore %var %uint_6 + OpReturn + + OpFunctionEnd + )")); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + FunctionEmitter fe(p, *spirv_function(100)); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(Assignment{ + Identifier{var} + ScalarConstructor{0} +} +Loop{ + Assignment{ + Identifier{var} + ScalarConstructor{1} + } + Assignment{ + Identifier{var} + ScalarConstructor{2} + } + If{ + ( + ScalarConstructor{true} + ) + { + Assignment{ + Identifier{var} + ScalarConstructor{3} + } + Continue{} + } + } + Assignment{ + Identifier{var} + ScalarConstructor{4} + } +} +Assignment{ + Identifier{var} + ScalarConstructor{6} +} +Return{} +)")) << ToString(fe.ast_body()); +} + TEST_F(SpvParserTest, EmitBody_BranchConditional_Continue_IfBreak_OnTrue) { auto* p = parser(test::Assemble(CommonTypes() + R"( %100 = OpFunction %void None %voidfn @@ -10747,10 +10767,6 @@ Loop{ } } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{4} @@ -10842,10 +10858,6 @@ Loop{ } } } - Else{ - { - } - } Assignment{ Identifier{var} ScalarConstructor{4} @@ -11061,10 +11073,6 @@ If{ { } } -Else{ - { - } -} Assignment{ Identifier{var} ScalarConstructor{5}