[spirv-reader] Emit break-if as needed.

This CL updates the SPIRV-Reader to emit `break-if` nodes instead of
`if-break` statements.

Bug: tint:1724
Change-Id: I8cd568f5e90a950acc5a42a470345273a5f1e6bc
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/111103
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
Reviewed-by: David Neto <dneto@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>

src/tint/reader/spirv/function.cc

@@ -3210,8 +3210,8 @@ bool FunctionEmitter::EmitNormalTerminator(const BlockInfo& block_info) {
                 return false;
             }
             AddStatement(create<ast::ReturnStatement>(Source{}, value.expr));
-        }
             return true;
+        }
         case spv::Op::OpKill:
             // For now, assume SPIR-V OpKill has same semantics as WGSL discard.
             // TODO(dneto): https://github.com/gpuweb/gpuweb/issues/676
@@ -3266,21 +3266,58 @@ bool FunctionEmitter::EmitNormalTerminator(const BlockInfo& block_info) {
                 return Fail() << "Fallthrough not supported in WGSL";
             }
 
+            // In the case of a continuing block a `break-if` needs to be emitted for either an
+            // if-break or an if-else-break statement. This only happens inside the continue block.
+            // It's possible for a continue block to also be the loop block, so checks are needed
+            // that this is a continue construct and the header construct will cause a continuing
+            // construct to be emitted. (i.e. the header is not `continue is entire loop`.
+            bool needs_break_if = false;
+            if ((true_kind == EdgeKind::kLoopBreak || false_kind == EdgeKind::kLoopBreak) &&
+                block_info.construct && block_info.construct->kind == Construct::Kind::kContinue) {
+                auto* header = GetBlockInfo(block_info.construct->begin_id);
+
+                TINT_ASSERT(Reader, header->construct &&
+                                        header->construct->kind == Construct::Kind::kContinue);
+                if (!header->is_continue_entire_loop) {
+                    needs_break_if = true;
+                }
+            }
+
             // At this point, at most one edge is kForward or kIfBreak.
 
-            // Emit an 'if' statement to express the *other* branch as a conditional
+            // If this is a continuing block and a `break` is to be emitted, then this needs to be
-            // break or continue.  Either or both of these could be nullptr.
+            // converted to a `break-if`. This may involve inverting the condition if this was a
-            // (A nullptr is generated for kIfBreak, kForward, or kBack.)
+            // `break-unless`.
-            // Also if one of the branches is an if-break out of an if-selection
+            if (needs_break_if) {
-            // requiring a flow guard, then get that flow guard name too.  It will
+                if (true_kind == EdgeKind::kLoopBreak && false_kind == EdgeKind::kLoopBreak) {
-            // come from at most one of these two branches.
+                    // Both branches break ... ?
-            std::string flow_guard;
+                    return Fail() << "Both branches of if inside continuing break.";
-            auto* true_branch = MakeBranchDetailed(block_info, *true_info, &flow_guard);
+                }
-            auto* false_branch = MakeBranchDetailed(block_info, *false_info, &flow_guard);
 
-            AddStatement(MakeSimpleIf(cond, true_branch, false_branch));
+                if (true_kind == EdgeKind::kLoopBreak) {
-            if (!flow_guard.empty()) {
+                    AddStatement(create<ast::BreakIfStatement>(Source{}, cond));
-                PushGuard(flow_guard, statements_stack_.Back().GetEndId());
+                } else {
+                    AddStatement(create<ast::BreakIfStatement>(
+                        Source{},
+                        create<ast::UnaryOpExpression>(Source{}, ast::UnaryOp::kNot, cond)));
+                }
+                return true;
+
+            } else {
+                // Emit an 'if' statement to express the *other* branch as a conditional
+                // break or continue.  Either or both of these could be nullptr.
+                // (A nullptr is generated for kIfBreak, kForward, or kBack.)
+                // Also if one of the branches is an if-break out of an if-selection
+                // requiring a flow guard, then get that flow guard name too.  It will
+                // come from at most one of these two branches.
+                std::string flow_guard;
+                auto* true_branch = MakeBranchDetailed(block_info, *true_info, &flow_guard);
+                auto* false_branch = MakeBranchDetailed(block_info, *false_info, &flow_guard);
+
+                AddStatement(MakeSimpleIf(cond, true_branch, false_branch));
+                if (!flow_guard.empty()) {
+                    PushGuard(flow_guard, statements_stack_.Back().GetEndId());
+                }
             }
             return true;
         }

src/tint/reader/spirv/function_cfg_test.cc

@@ -638,9 +638,8 @@ TEST_F(SpvParserCFGTest, RegisterMerges_GoodLoopMerge_MultiBlockLoop_ContinueIsN
     EXPECT_FALSE(bi99->is_continue_entire_loop);
 }
 
-TEST_F(
+TEST_F(SpvParserCFGTest,
-    SpvParserCFGTest,
+       RegisterMerges_GoodLoopMerge_MultiBlockLoop_ContinueIsNotHeader_BranchConditional) {
-    RegisterMerges_GoodLoopMerge_MultiBlockLoop_ContinueIsNotHeader_BranchConditional) {  // NOLINT
     auto p = parser(test::Assemble(CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -2605,7 +2604,7 @@ TEST_F(SpvParserCFGTest, VerifyHeaderContinueMergeOrder_HeaderDoesNotStrictlyDom
 }
 
 TEST_F(SpvParserCFGTest,
-       VerifyHeaderContinueMergeOrder_HeaderDoesNotStrictlyDominateContinueTarget) {  // NOLINT
+       VerifyHeaderContinueMergeOrder_HeaderDoesNotStrictlyDominateContinueTarget) {
     auto assembly = CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -4451,9 +4450,8 @@ TEST_F(SpvParserCFGTest, ClassifyCFGEdges_BackEdge_MultiBlockLoop_SingleBlockCon
     EXPECT_EQ(bi40->succ_edge[20], EdgeKind::kBack);
 }
 
-TEST_F(
+TEST_F(SpvParserCFGTest,
-    SpvParserCFGTest,
+       ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsNotHeader) {
-    ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsNotHeader) {  // NOLINT
     auto assembly = CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -4489,9 +4487,8 @@ TEST_F(
     EXPECT_EQ(bi50->succ_edge[20], EdgeKind::kBack);
 }
 
-TEST_F(
+TEST_F(SpvParserCFGTest,
-    SpvParserCFGTest,
+       ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsHeader) {
-    ClassifyCFGEdges_BackEdge_MultiBlockLoop_MultiBlockContinueConstruct_ContinueIsHeader) {  // NOLINT
     auto assembly = CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -6051,7 +6048,7 @@ TEST_F(SpvParserCFGTest, ClassifyCFGEdges_DomViolation_AfterContinueToContinueIn
 }
 
 TEST_F(SpvParserCFGTest,
-       FindSwitchCaseHeaders_DomViolation_SwitchCase_CantBeMergeForOtherConstruct) {  // NOLINT
+       FindSwitchCaseHeaders_DomViolation_SwitchCase_CantBeMergeForOtherConstruct) {
     auto assembly = CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -6762,8 +6759,7 @@ TEST_F(SpvParserCFGTest, ClassifyCFGEdges_IfBreak_WithForwardToPremerge) {
 }
 
 TEST_F(SpvParserCFGTest,
-       FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeTrueHeader) {  // NOLINT -
+       FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeTrueHeader) {
-                                                                                      // line length
     auto assembly = CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -6795,10 +6791,8 @@ TEST_F(SpvParserCFGTest,
                    "merge block for header block 20 (violates dominance rule)"));
 }
 
-TEST_F(
+TEST_F(SpvParserCFGTest,
-    SpvParserCFGTest,
+       FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeFalseHeader) {
-    FindIfSelectionInternalHeaders_DomViolation_InteriorMerge_CantBeFalseHeader) {  // NOLINT - line
-                                                                                    // length
     auto assembly = CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -9474,9 +9468,7 @@ TEST_F(SpvParserCFGTest,
 }
 
 TEST_F(SpvParserCFGTest,
-       EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Unconditional) {  // NOLINT
+       EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Unconditional) {
-                                                                                           // - line
-                                                                                           // length
     auto p = parser(test::Assemble(CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -9519,10 +9511,46 @@ return;
     ASSERT_EQ(expect, got);
 }
 
-TEST_F(
+TEST_F(SpvParserCFGTest,
-    SpvParserCFGTest,
+       EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Conditional_BreakIf) {
-    EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Conditional) {  // NOLINT -
+    auto p = parser(test::Assemble(CommonTypes() + R"(
-                                                                                      // line length
+     %100 = OpFunction %void None %voidfn
+
+     %10 = OpLabel
+     OpBranch %20
+
+     %20 = OpLabel
+     OpLoopMerge %99 %80 None
+     OpBranch %80
+
+     %80 = OpLabel ; continue target
+     OpStore %var %uint_1
+     OpBranchConditional %cond %99 %20  ; exit, and backedge
+
+     %99 = OpLabel
+     OpReturn
+
+     OpFunctionEnd
+  )"));
+    ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+    auto fe = p->function_emitter(100);
+    EXPECT_TRUE(fe.EmitBody()) << p->error();
+    auto ast_body = fe.ast_body();
+    auto got = test::ToString(p->program(), ast_body);
+    auto* expect = R"(loop {
+
+  continuing {
+    var_1 = 1u;
+    break if false;
+  }
+}
+return;
+)";
+    ASSERT_EQ(expect, got);
+}
+
+TEST_F(SpvParserCFGTest,
+       EmitBody_Branch_LoopBreak_MultiBlockLoop_FromContinueConstructEnd_Conditional) {
     auto p = parser(test::Assemble(CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn
 
@@ -9551,10 +9579,48 @@ TEST_F(
 
   continuing {
     var_1 = 1u;
-    if (false) {
+    break if !(false);
-    } else {
+  }
-      break;
+}
-    }
+return;
+)";
+    ASSERT_EQ(expect, got);
+}
+
+TEST_F(SpvParserCFGTest, EmitBody_Branch_LoopBreak_FromContinueConstructTail) {
+    auto p = parser(test::Assemble(CommonTypes() + R"(
+     %100 = OpFunction %void None %voidfn
+
+     %10 = OpLabel
+     OpBranch %20
+
+     %20 = OpLabel
+     OpLoopMerge %99 %50 None
+     OpBranchConditional %cond %30 %99
+     %30 = OpLabel
+     OpBranch %50
+     %50 = OpLabel
+     OpBranch %60
+     %60 = OpLabel
+     OpBranchConditional %cond %20 %99
+     %99 = OpLabel
+     OpReturn
+
+     OpFunctionEnd
+  )"));
+    ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+    auto fe = p->function_emitter(100);
+    EXPECT_TRUE(fe.EmitBody()) << p->error();
+    auto ast_body = fe.ast_body();
+    auto got = test::ToString(p->program(), ast_body);
+    auto* expect = R"(loop {
+  if (false) {
+  } else {
+    break;
+  }
+
+  continuing {
+    break if !(false);
   }
 }
 return;
@@ -10009,9 +10075,7 @@ loop {
   var_1 = 1u;
 
   continuing {
-    if (false) {
+    break if false;
-      break;
-    }
   }
 }
 var_1 = 5u;
@@ -10052,10 +10116,7 @@ loop {
   var_1 = 1u;
 
   continuing {
-    if (false) {
+    break if !(false);
-    } else {
-      break;
-    }
   }
 }
 var_1 = 5u;
@@ -10878,9 +10939,8 @@ return;
     ASSERT_EQ(expect, got);
 }
 
-TEST_F(
+TEST_F(SpvParserCFGTest,
-    SpvParserCFGTest,
+       EmitBody_BranchConditional_Continue_Continue_AfterHeader_Conditional_EmptyContinuing) {
-    EmitBody_BranchConditional_Continue_Continue_AfterHeader_Conditional_EmptyContinuing) {  // NOLINT
     // Like the previous tests, but with an empty continuing clause.
     auto p = parser(test::Assemble(CommonTypes() + R"(
      %100 = OpFunction %void None %voidfn

src/tint/reader/spirv/function_var_test.cc

@@ -689,9 +689,7 @@ loop {
 
   continuing {
     x_1 = 4u;
-    if (false) {
+    break if false;
-      break;
-    }
   }
 }
 x_1 = 5u;
@@ -1610,9 +1608,7 @@ OpFunctionEnd
     x_999 = false;
 
     continuing {
-      if (true) {
+      break if true;
-        break;
-      }
     }
   }
 }
@@ -1694,9 +1690,7 @@ loop {
     x_999 = false;
 
     continuing {
-      if (true) {
+      break if true;
-        break;
-      }
     }
   }
 

test/tint/vk-gl-cts/graphicsfuzz/write-red-after-search/0-opt.wgsl

@@ -103,10 +103,7 @@ fn insert_i1_i1_(treeIndex : ptr<function, i32>, data_1 : ptr<function, i32>) {
           continuing {
             let x_382 : f32 = x_27.injectionSwitch.x;
             let x_384 : f32 = x_27.injectionSwitch.y;
-            if ((x_382 > x_384)) {
+            break if !(x_382 > x_384);
-            } else {
-              break;
-            }
           }
         }
         continue;