HLSL: workaround FXC error "continue cannot be used in a switch"

Added a new transform::RemoveContinueInSwitch that replaces continue
statements in switch cases with setting a bool variable, and checking if
the variable is set after the switch to continue.

Bug: tint:1080
Change-Id: I3c0a6c790e1bb612fac3f927a4bd5beb2d0d4ed1
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/84960
Reviewed-by: James Price <jrprice@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>

src/tint/BUILD.gn

@@ -471,6 +471,8 @@ libtint_source_set("libtint_core_all_src") {
     "transform/promote_initializers_to_const_var.h",
     "transform/promote_side_effects_to_decl.cc",
     "transform/promote_side_effects_to_decl.h",
+    "transform/remove_continue_in_switch.cc",
+    "transform/remove_continue_in_switch.h",
     "transform/remove_phonies.cc",
     "transform/remove_phonies.h",
     "transform/remove_unreachable_statements.cc",

src/tint/CMakeLists.txt

@@ -351,6 +351,8 @@ set(TINT_LIB_SRCS
   transform/promote_side_effects_to_decl.h
   transform/remove_phonies.cc
   transform/remove_phonies.h
+  transform/remove_continue_in_switch.cc
+  transform/remove_continue_in_switch.h
   transform/remove_unreachable_statements.cc
   transform/remove_unreachable_statements.h
   transform/renamer.cc
@@ -1027,6 +1029,7 @@ if(TINT_BUILD_TESTS)
       transform/num_workgroups_from_uniform_test.cc
       transform/promote_initializers_to_const_var_test.cc
       transform/promote_side_effects_to_decl_test.cc
+      transform/remove_continue_in_switch_test.cc
       transform/remove_phonies_test.cc
       transform/remove_unreachable_statements_test.cc
       transform/renamer_test.cc

src/tint/transform/remove_continue_in_switch.cc

@@ -0,0 +1,145 @@
+// Copyright 2022 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/tint/transform/remove_continue_in_switch.h"
+
+#include <string>
+#include <unordered_map>
+#include <utility>
+
+#include "src/tint/ast/continue_statement.h"
+#include "src/tint/ast/switch_statement.h"
+#include "src/tint/program_builder.h"
+#include "src/tint/sem/block_statement.h"
+#include "src/tint/sem/for_loop_statement.h"
+#include "src/tint/sem/loop_statement.h"
+#include "src/tint/sem/switch_statement.h"
+#include "src/tint/transform/utils/get_insertion_point.h"
+#include "src/tint/utils/map.h"
+
+TINT_INSTANTIATE_TYPEINFO(tint::transform::RemoveContinueInSwitch);
+
+namespace tint::transform {
+namespace {
+
+class State {
+ private:
+  CloneContext& ctx;
+  ProgramBuilder& b;
+  const sem::Info& sem;
+
+  // Map of switch statement to 'tint_continue' variable.
+  std::unordered_map<const ast::SwitchStatement*, Symbol>
+      switch_to_cont_var_name;
+
+  // If `cont` is within a switch statement within a loop, returns a pointer to
+  // that switch statement.
+  static const ast::SwitchStatement* GetParentSwitchInLoop(
+      const sem::Info& sem,
+      const ast::ContinueStatement* cont) {
+    // Find whether first parent is a switch or a loop
+    auto* sem_stmt = sem.Get(cont);
+    auto* sem_parent =
+        sem_stmt->FindFirstParent<sem::SwitchStatement, sem::LoopBlockStatement,
+                                  sem::ForLoopStatement>();
+    if (!sem_parent) {
+      return nullptr;
+    }
+    return sem_parent->Declaration()->As<ast::SwitchStatement>();
+  }
+
+ public:
+  /// Constructor
+  /// @param ctx_in the context
+  explicit State(CloneContext& ctx_in)
+      : ctx(ctx_in), b(*ctx_in.dst), sem(ctx_in.src->Sem()) {}
+
+  /// Returns true if this transform should be run for the given program
+  static bool ShouldRun(const Program* program) {
+    for (auto* node : program->ASTNodes().Objects()) {
+      auto* stmt = node->As<ast::ContinueStatement>();
+      if (!stmt) {
+        continue;
+      }
+      if (GetParentSwitchInLoop(program->Sem(), stmt)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /// Runs the transform
+  void Run() {
+    for (auto* node : ctx.src->ASTNodes().Objects()) {
+      auto* cont = node->As<ast::ContinueStatement>();
+      if (!cont) {
+        continue;
+      }
+
+      // If first parent is not a switch within a loop, skip
+      auto* switch_stmt = GetParentSwitchInLoop(sem, cont);
+      if (!switch_stmt) {
+        continue;
+      }
+
+      auto cont_var_name =
+          tint::utils::GetOrCreate(switch_to_cont_var_name, switch_stmt, [&]() {
+            // Create and insert 'var tint_continue : bool = false;' before the
+            // switch.
+            auto var_name = b.Symbols().New("tint_continue");
+            auto* decl = b.Decl(b.Var(var_name, b.ty.bool_(), b.Expr(false)));
+            auto ip = utils::GetInsertionPoint(ctx, switch_stmt);
+            ctx.InsertBefore(ip.first->Declaration()->statements, ip.second,
+                             decl);
+
+            // Create and insert 'if (tint_continue) { continue; }' after
+            // switch.
+            auto* if_stmt = b.If(b.Expr(var_name), b.Block(b.Continue()));
+            ctx.InsertAfter(ip.first->Declaration()->statements, ip.second,
+                            if_stmt);
+
+            // Return the new var name
+            return var_name;
+          });
+
+      // Replace 'continue;' with '{ tint_continue = true; break; }'
+      auto* new_stmt = b.Block(                   //
+          b.Assign(b.Expr(cont_var_name), true),  //
+          b.Break());
+
+      ctx.Replace(cont, new_stmt);
+    }
+
+    ctx.Clone();
+  }
+};
+
+}  // namespace
+
+RemoveContinueInSwitch::RemoveContinueInSwitch() = default;
+RemoveContinueInSwitch::~RemoveContinueInSwitch() = default;
+
+bool RemoveContinueInSwitch::ShouldRun(const Program* program,
+                                       const DataMap& /*data*/) const {
+  return State::ShouldRun(program);
+}
+
+void RemoveContinueInSwitch::Run(CloneContext& ctx,
+                                 const DataMap&,
+                                 DataMap&) const {
+  State state(ctx);
+  state.Run();
+}
+
+}  // namespace tint::transform

src/tint/transform/remove_continue_in_switch.h

@@ -0,0 +1,55 @@
+// Copyright 2022 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef SRC_TINT_TRANSFORM_REMOVE_CONTINUE_IN_SWITCH_H_
+#define SRC_TINT_TRANSFORM_REMOVE_CONTINUE_IN_SWITCH_H_
+
+#include "src/tint/transform/transform.h"
+
+namespace tint::transform {
+
+/// This transform replaces continue statements in switch cases with setting a
+/// bool variable, and checking if the variable is set after the switch to
+/// continue. It is necessary to work around FXC "error X3708: continue cannot
+/// be used in a switch". See crbug.com/tint/1080.
+class RemoveContinueInSwitch
+    : public Castable<RemoveContinueInSwitch, Transform> {
+ public:
+  /// Constructor
+  RemoveContinueInSwitch();
+
+  /// Destructor
+  ~RemoveContinueInSwitch() override;
+
+ protected:
+  /// @param program the program to inspect
+  /// @param data optional extra transform-specific input data
+  /// @returns true if this transform should be run for the given program
+  bool ShouldRun(const Program* program,
+                 const DataMap& data = {}) const override;
+
+  /// Runs the transform using the CloneContext built for transforming a
+  /// program. Run() is responsible for calling Clone() on the CloneContext.
+  /// @param ctx the CloneContext primed with the input program and
+  /// ProgramBuilder
+  /// @param inputs optional extra transform-specific input data
+  /// @param outputs optional extra transform-specific output data
+  void Run(CloneContext& ctx,
+           const DataMap& inputs,
+           DataMap& outputs) const override;
+};
+
+}  // namespace tint::transform
+
+#endif  // SRC_TINT_TRANSFORM_REMOVE_CONTINUE_IN_SWITCH_H_

src/tint/transform/remove_continue_in_switch_test.cc

@@ -0,0 +1,565 @@
+// Copyright 2022 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/tint/transform/remove_continue_in_switch.h"
+#include "src/tint/transform/test_helper.h"
+
+namespace tint {
+namespace transform {
+namespace {
+
+using RemoveContinueInSwitchTest = TransformTest;
+
+TEST_F(RemoveContinueInSwitchTest, ShouldRun_True) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  loop {
+    switch(i) {
+      case 0: {
+        continue;
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    break;
+  }
+}
+)";
+
+  EXPECT_TRUE(ShouldRun<RemoveContinueInSwitch>(src));
+}
+
+TEST_F(RemoveContinueInSwitchTest, ShouldRunEmptyModule_False) {
+  auto* src = "";
+
+  EXPECT_FALSE(ShouldRun<RemoveContinueInSwitch>(src));
+}
+
+TEST_F(RemoveContinueInSwitchTest, ShouldRunContinueNotInSwitch_False) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  loop {
+    switch(i) {
+      case 0: {
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+
+    if (true) {
+      continue;
+    }
+    break;
+  }
+}
+)";
+
+  EXPECT_FALSE(ShouldRun<RemoveContinueInSwitch>(src));
+}
+
+TEST_F(RemoveContinueInSwitchTest, ShouldRunContinueInLoopInSwitch_False) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  switch(i) {
+    case 0: {
+      loop {
+        if (true) {
+          continue;
+        }
+        break;
+      }
+      break;
+    }
+    default: {
+      break;
+    }
+  }
+}
+)";
+
+  EXPECT_FALSE(ShouldRun<RemoveContinueInSwitch>(src));
+}
+
+TEST_F(RemoveContinueInSwitchTest, EmptyModule) {
+  auto* src = "";
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<RemoveContinueInSwitch>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(RemoveContinueInSwitchTest, SingleContinue) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    switch(i) {
+      case 0: {
+        continue;
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    let marker2 = 0;
+    break;
+
+    continuing {
+      let marker3 = 0;
+    }
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    var tint_continue : bool = false;
+    switch(i) {
+      case 0: {
+        {
+          tint_continue = true;
+          break;
+        }
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    if (tint_continue) {
+      continue;
+    }
+    let marker2 = 0;
+    break;
+
+    continuing {
+      let marker3 = 0;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<RemoveContinueInSwitch>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(RemoveContinueInSwitchTest, MultipleContinues) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    switch(i) {
+      case 0: {
+        continue;
+        break;
+      }
+      case 1: {
+        continue;
+        break;
+      }
+      case 2: {
+        continue;
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    let marker2 = 0;
+    break;
+
+    continuing {
+      let marker3 = 0;
+    }
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    var tint_continue : bool = false;
+    switch(i) {
+      case 0: {
+        {
+          tint_continue = true;
+          break;
+        }
+        break;
+      }
+      case 1: {
+        {
+          tint_continue = true;
+          break;
+        }
+        break;
+      }
+      case 2: {
+        {
+          tint_continue = true;
+          break;
+        }
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    if (tint_continue) {
+      continue;
+    }
+    let marker2 = 0;
+    break;
+
+    continuing {
+      let marker3 = 0;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<RemoveContinueInSwitch>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(RemoveContinueInSwitchTest, MultipleSwitch) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    switch(i) {
+      case 0: {
+        continue;
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    let marker2 = 0;
+
+    let marker3 = 0;
+    switch(i) {
+      case 0: {
+        continue;
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    let marker4 = 0;
+
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    var tint_continue : bool = false;
+    switch(i) {
+      case 0: {
+        {
+          tint_continue = true;
+          break;
+        }
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    if (tint_continue) {
+      continue;
+    }
+    let marker2 = 0;
+    let marker3 = 0;
+    var tint_continue_1 : bool = false;
+    switch(i) {
+      case 0: {
+        {
+          tint_continue_1 = true;
+          break;
+        }
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    if (tint_continue_1) {
+      continue;
+    }
+    let marker4 = 0;
+    break;
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<RemoveContinueInSwitch>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(RemoveContinueInSwitchTest, NestedLoopSwitch) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    switch(i) {
+      case 0: {
+        var j = 0;
+        loop {
+          let marker3 = 0;
+          switch(j) {
+            case 0: {
+              continue;
+              break;
+            }
+            default: {
+              break;
+            }
+          }
+          let marker4 = 0;
+          break;
+        }
+        continue;
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    let marker2 = 0;
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i = 0;
+  loop {
+    let marker1 = 0;
+    var tint_continue_1 : bool = false;
+    switch(i) {
+      case 0: {
+        var j = 0;
+        loop {
+          let marker3 = 0;
+          var tint_continue : bool = false;
+          switch(j) {
+            case 0: {
+              {
+                tint_continue = true;
+                break;
+              }
+              break;
+            }
+            default: {
+              break;
+            }
+          }
+          if (tint_continue) {
+            continue;
+          }
+          let marker4 = 0;
+          break;
+        }
+        {
+          tint_continue_1 = true;
+          break;
+        }
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    if (tint_continue_1) {
+      continue;
+    }
+    let marker2 = 0;
+    break;
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<RemoveContinueInSwitch>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(RemoveContinueInSwitchTest, ExtraScopes) {
+  auto* src = R"(
+fn f() {
+  var i = 0;
+  var a = true;
+  var b = true;
+  var c = true;
+  var d = true;
+  loop {
+    if (a) {
+      if (b) {
+        let marker1 = 0;
+        switch(i) {
+          case 0: {
+            if (c) {
+              if (d) {
+                continue;
+              }
+            }
+            break;
+          }
+          default: {
+            break;
+          }
+        }
+        let marker2 = 0;
+        break;
+      }
+    }
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i = 0;
+  var a = true;
+  var b = true;
+  var c = true;
+  var d = true;
+  loop {
+    if (a) {
+      if (b) {
+        let marker1 = 0;
+        var tint_continue : bool = false;
+        switch(i) {
+          case 0: {
+            if (c) {
+              if (d) {
+                {
+                  tint_continue = true;
+                  break;
+                }
+              }
+            }
+            break;
+          }
+          default: {
+            break;
+          }
+        }
+        if (tint_continue) {
+          continue;
+        }
+        let marker2 = 0;
+        break;
+      }
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<RemoveContinueInSwitch>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(RemoveContinueInSwitchTest, ForLoop) {
+  auto* src = R"(
+fn f() {
+  for (var i = 0; i < 4; i = i + 1) {
+    let marker1 = 0;
+    switch(i) {
+      case 0: {
+        continue;
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    let marker2 = 0;
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  for(var i = 0; (i < 4); i = (i + 1)) {
+    let marker1 = 0;
+    var tint_continue : bool = false;
+    switch(i) {
+      case 0: {
+        {
+          tint_continue = true;
+          break;
+        }
+        break;
+      }
+      default: {
+        break;
+      }
+    }
+    if (tint_continue) {
+      continue;
+    }
+    let marker2 = 0;
+    break;
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<RemoveContinueInSwitch>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+}  // namespace
+}  // namespace transform
+}  // namespace tint

src/tint/writer/hlsl/generator_impl.cc

@@ -59,6 +59,7 @@
 #include "src/tint/transform/num_workgroups_from_uniform.h"
 #include "src/tint/transform/promote_initializers_to_const_var.h"
 #include "src/tint/transform/promote_side_effects_to_decl.h"
+#include "src/tint/transform/remove_continue_in_switch.h"
 #include "src/tint/transform/remove_phonies.h"
 #include "src/tint/transform/simplify_pointers.h"
 #include "src/tint/transform/unshadow.h"
@@ -210,6 +211,8 @@ SanitizedResult Sanitize(
   manager.Add<transform::CalculateArrayLength>();
   manager.Add<transform::PromoteInitializersToConstVar>();
 
+  manager.Add<transform::RemoveContinueInSwitch>();
+
   manager.Add<transform::AddEmptyEntryPoint>();
 
   data.Add<transform::CanonicalizeEntryPointIO::Config>(