[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 <dsinclair@google.com>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 0cd954e..6203a5d 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -1790,12 +1790,15 @@
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<ast::ElseStatement>(
- 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<ast::ElseStatement>(
+ 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 52b42f1..4150cae 100644
--- a/src/reader/spirv/function_cfg_test.cc
+++ b/src/reader/spirv/function_cfg_test.cc
@@ -7406,10 +7406,6 @@
{
}
}
-Else{
- {
- }
-}
Return{}
)"));
}
@@ -7452,10 +7448,6 @@
}
}
}
-Else{
- {
- }
-}
Assignment{
Identifier{var}
ScalarConstructor{999}
@@ -7684,10 +7676,6 @@
}
}
}
-Else{
- {
- }
-}
Assignment{
Identifier{var}
ScalarConstructor{3}
@@ -7828,10 +7816,6 @@
}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{3}
@@ -8252,10 +8236,6 @@
}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{5}
@@ -8504,10 +8484,6 @@
Continue{}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{2}
@@ -8849,10 +8825,6 @@
Return{}
}
}
-Else{
- {
- }
-}
Return{}
)")) << ToString(fe.ast_body());
}
@@ -8960,10 +8932,6 @@
}
}
}
-Else{
- {
- }
-}
Return{
{
ScalarConstructor{3}
@@ -9067,10 +9035,6 @@
Kill{}
}
}
-Else{
- {
- }
-}
Kill{}
)")) << ToString(fe.ast_body());
}
@@ -9153,10 +9117,6 @@
Return{}
}
}
-Else{
- {
- }
-}
Return{}
)")) << ToString(fe.ast_body());
}
@@ -9507,10 +9467,6 @@
Continue{}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{2}
@@ -9559,10 +9515,6 @@
}
}
}
-Else{
- {
- }
-}
Assignment{
Identifier{var}
ScalarConstructor{2}
@@ -10188,10 +10140,6 @@
}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{3}
@@ -10286,10 +10234,6 @@
}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{3}
@@ -10648,10 +10592,6 @@
Continue{}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{4}
@@ -10671,6 +10611,86 @@
)")) << 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 @@
}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{4}
@@ -10842,10 +10858,6 @@
}
}
}
- Else{
- {
- }
- }
Assignment{
Identifier{var}
ScalarConstructor{4}
@@ -11061,10 +11073,6 @@
{
}
}
-Else{
- {
- }
-}
Assignment{
Identifier{var}
ScalarConstructor{5}
