writer/msl: Fix continuing block emission

Inline the `continuing` block in the places where `continue` is called.

Simplifies the emission, and fixes emission of `let` statements in the loop.

This fix matches the same approach in writer/hlsl.
See: https://dawn-review.googlesource.com/c/tint/+/51784

Fixed: tint:833
Fixed: tint:914
Change-Id: If4d8cde62dfaf8efa24272854ca7ff5edc0a8234
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/55341
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: David Neto <dneto@google.com>

src/writer/msl/generator_impl.cc

@@ -50,6 +50,7 @@
 #include "src/sem/variable.h"
 #include "src/sem/vector_type.h"
 #include "src/sem/void_type.h"
+#include "src/utils/scoped_assignment.h"
 #include "src/writer/float_to_string.h"
 
 namespace tint {
@@ -875,6 +876,9 @@ bool GeneratorImpl::EmitConstructor(ast::ConstructorExpression* expr) {
 }
 
 bool GeneratorImpl::EmitContinue(ast::ContinueStatement*) {
+  if (!emit_continuing_()) {
+    return false;
+  }
   make_indent();
   out_ << "continue;" << std::endl;
   return true;
@@ -1279,91 +1283,30 @@ bool GeneratorImpl::EmitIdentifier(ast::IdentifierExpression* expr) {
 }
 
 bool GeneratorImpl::EmitLoop(ast::LoopStatement* stmt) {
-  loop_emission_counter_++;
-
-  std::string guard =
-      "tint_msl_is_first_" + std::to_string(loop_emission_counter_);
-
-  if (stmt->has_continuing()) {
-    make_indent();
-
-    // Continuing variables get their own scope.
-    out_ << "{" << std::endl;
-    increment_indent();
-
-    make_indent();
-    out_ << "bool " << guard << " = true;" << std::endl;
-
-    // A continuing block may use variables declared in the method body. As a
-    // first pass, if we have a continuing, we pull all declarations outside
-    // the for loop into the continuing scope. Then, the variable declarations
-    // will be turned into assignments.
-    for (auto* s : *(stmt->body())) {
-      if (auto* decl = s->As<ast::VariableDeclStatement>()) {
-        if (!EmitVariable(program_->Sem().Get(decl->variable()), true)) {
-          return false;
-        }
-      }
-    }
-  }
-
   make_indent();
-  out_ << "for(;;) {" << std::endl;
-  increment_indent();
 
-  if (stmt->has_continuing()) {
-    make_indent();
-    out_ << "if (!" << guard << ") ";
-
-    if (!EmitBlockAndNewline(stmt->continuing())) {
-      return false;
-    }
-
-    make_indent();
-    out_ << guard << " = false;" << std::endl;
-    out_ << std::endl;
-  }
-
-  for (auto* s : *(stmt->body())) {
-    // If we have a continuing block we've already emitted the variable
-    // declaration before the loop, so treat it as an assignment.
-    auto* decl = s->As<ast::VariableDeclStatement>();
-    if (decl != nullptr && stmt->has_continuing()) {
+  auto emit_continuing = [this, stmt]() {
+    if (stmt->has_continuing()) {
       make_indent();
-
-      auto* var = decl->variable();
-      out_ << program_->Symbols().NameFor(var->symbol()) << " = ";
-      if (var->constructor() != nullptr) {
-        if (!EmitExpression(var->constructor())) {
-          return false;
-        }
-      } else {
-        auto* type = program_->Sem().Get(var)->Type()->UnwrapRef();
-        if (!EmitZeroValue(type)) {
-          return false;
-        }
+      if (!EmitBlock(stmt->continuing())) {
+        return false;
       }
-      out_ << ";" << std::endl;
-      continue;
+      out_ << std::endl;
     }
+    return true;
+  };
 
-    if (!EmitStatement(s)) {
-      return false;
+  TINT_SCOPED_ASSIGNMENT(emit_continuing_, emit_continuing);
+  bool ok = EmitBlockBraces("while (true)", [&] {
+    for (auto* s : stmt->body()->statements()) {
+      if (!EmitStatement(s)) {
+        return false;
+      }
     }
-  }
-
-  decrement_indent();
-  make_indent();
-  out_ << "}" << std::endl;
-
-  // Close the scope for any continuing variables.
-  if (stmt->has_continuing()) {
-    decrement_indent();
-    make_indent();
-    out_ << "}" << std::endl;
-  }
-
-  return true;
+    return emit_continuing();
+  });
+  out_ << std::endl;
+  return ok;
 }
 
 bool GeneratorImpl::EmitDiscard(ast::DiscardStatement*) {
@@ -1530,7 +1473,7 @@ bool GeneratorImpl::EmitStatement(ast::Statement* stmt) {
   }
   if (auto* v = stmt->As<ast::VariableDeclStatement>()) {
     auto* var = program_->Sem().Get(v->variable());
-    return EmitVariable(var, false);
+    return EmitVariable(var);
   }
 
   diagnostics_.add_error("unknown statement type: " + program_->str(stmt));
@@ -1882,8 +1825,7 @@ bool GeneratorImpl::EmitUnaryOp(ast::UnaryOpExpression* expr) {
   return true;
 }
 
-bool GeneratorImpl::EmitVariable(const sem::Variable* var,
-                                 bool skip_constructor) {
+bool GeneratorImpl::EmitVariable(const sem::Variable* var) {
   make_indent();
 
   auto* decl = var->Declaration();
@@ -1925,19 +1867,17 @@ bool GeneratorImpl::EmitVariable(const sem::Variable* var,
     out_ << " " << name;
   }
 
-  if (!skip_constructor) {
-    if (decl->constructor() != nullptr) {
-      out_ << " = ";
-      if (!EmitExpression(decl->constructor())) {
-        return false;
-      }
-    } else if (var->StorageClass() == ast::StorageClass::kPrivate ||
-               var->StorageClass() == ast::StorageClass::kFunction ||
-               var->StorageClass() == ast::StorageClass::kNone) {
-      out_ << " = ";
-      if (!EmitZeroValue(type)) {
-        return false;
-      }
+  if (decl->constructor() != nullptr) {
+    out_ << " = ";
+    if (!EmitExpression(decl->constructor())) {
+      return false;
+    }
+  } else if (var->StorageClass() == ast::StorageClass::kPrivate ||
+             var->StorageClass() == ast::StorageClass::kFunction ||
+             var->StorageClass() == ast::StorageClass::kNone) {
+    out_ << " = ";
+    if (!EmitZeroValue(type)) {
+      return false;
     }
   }
   out_ << ";" << std::endl;
@@ -2046,6 +1986,21 @@ GeneratorImpl::SizeAndAlign GeneratorImpl::MslPackedTypeSizeAndAlign(
   return {};
 }
 
+template <typename F>
+bool GeneratorImpl::EmitBlockBraces(const std::string& prefix, F&& cb) {
+  out_ << prefix << (prefix.empty() ? "{" : " {") << std::endl;
+  increment_indent();
+
+  if (!cb()) {
+    return false;
+  }
+
+  decrement_indent();
+  make_indent();
+  out_ << "}";
+  return true;
+}
+
 }  // namespace msl
 }  // namespace writer
 }  // namespace tint

src/writer/msl/generator_impl.h

@@ -209,9 +209,8 @@ class GeneratorImpl : public TextGenerator {
   bool EmitUnaryOp(ast::UnaryOpExpression* expr);
   /// Handles generating a variable
   /// @param var the variable to generate
-  /// @param skip_constructor set true if the constructor should be skipped
   /// @returns true if the variable was emitted
-  bool EmitVariable(const sem::Variable* var, bool skip_constructor);
+  bool EmitVariable(const sem::Variable* var);
   /// Handles generating a program scope constant variable
   /// @param var the variable to emit
   /// @returns true if the variable was emitted
@@ -260,8 +259,17 @@ class GeneratorImpl : public TextGenerator {
   /// type.
   SizeAndAlign MslPackedTypeSizeAndAlign(const sem::Type* ty);
 
+  /// Emits `prefix`, followed by an opening brace `{`, then calls `cb` to emit
+  /// the block body, then finally emits the closing brace `}`.
+  /// @param prefix the string to emit before the opening brace
+  /// @param cb a function or function-like object with the signature `bool()`
+  /// that emits the block body.
+  /// @returns the return value of `cb`.
+  template <typename F>
+  bool EmitBlockBraces(const std::string& prefix, F&& cb);
+
   const Program* program_ = nullptr;
-  uint32_t loop_emission_counter_ = 0;
+  std::function<bool()> emit_continuing_;
 };
 
 }  // namespace msl

src/writer/msl/generator_impl_continue_test.cc

@@ -22,15 +22,18 @@ namespace {
 using MslGeneratorImplTest = TestHelper;
 
 TEST_F(MslGeneratorImplTest, Emit_Continue) {
-  auto* c = create<ast::ContinueStatement>();
-  WrapInFunction(Loop(Block(c)));
+  auto* loop = Loop(Block(create<ast::ContinueStatement>()));
+  WrapInFunction(loop);
 
   GeneratorImpl& gen = Build();
 
   gen.increment_indent();
 
-  ASSERT_TRUE(gen.EmitStatement(c)) << gen.error();
-  EXPECT_EQ(gen.result(), "  continue;\n");
+  ASSERT_TRUE(gen.EmitStatement(loop)) << gen.error();
+  EXPECT_EQ(gen.result(), R"(  while (true) {
+    continue;
+  }
+)");
 }
 
 }  // namespace

src/writer/msl/generator_impl_loop_test.cc

@@ -33,7 +33,7 @@ TEST_F(MslGeneratorImplTest, Emit_Loop) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(l)) << gen.error();
-  EXPECT_EQ(gen.result(), R"(  for(;;) {
+  EXPECT_EQ(gen.result(), R"(  while (true) {
     discard_fragment();
   }
 )");
@@ -50,15 +50,10 @@ TEST_F(MslGeneratorImplTest, Emit_LoopWithContinuing) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(l)) << gen.error();
-  EXPECT_EQ(gen.result(), R"(  {
-    bool tint_msl_is_first_1 = true;
-    for(;;) {
-      if (!tint_msl_is_first_1) {
-        return;
-      }
-      tint_msl_is_first_1 = false;
-
-      discard_fragment();
+  EXPECT_EQ(gen.result(), R"(  while (true) {
+    discard_fragment();
+    {
+      return;
     }
   }
 )");
@@ -84,26 +79,16 @@ TEST_F(MslGeneratorImplTest, Emit_LoopNestedWithContinuing) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(outer)) << gen.error();
-  EXPECT_EQ(gen.result(), R"(  {
-    bool tint_msl_is_first_1 = true;
-    for(;;) {
-      if (!tint_msl_is_first_1) {
-        lhs = rhs;
-      }
-      tint_msl_is_first_1 = false;
-
+  EXPECT_EQ(gen.result(), R"(  while (true) {
+    while (true) {
+      discard_fragment();
       {
-        bool tint_msl_is_first_2 = true;
-        for(;;) {
-          if (!tint_msl_is_first_2) {
-            return;
-          }
-          tint_msl_is_first_2 = false;
-
-          discard_fragment();
-        }
+        return;
       }
     }
+    {
+      lhs = rhs;
+    }
   }
 )");
 }
@@ -146,18 +131,11 @@ TEST_F(MslGeneratorImplTest, Emit_LoopWithVarUsedInContinuing) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(outer)) << gen.error();
-  EXPECT_EQ(gen.result(), R"(  {
-    bool tint_msl_is_first_1 = true;
-    float lhs;
-    float other;
-    for(;;) {
-      if (!tint_msl_is_first_1) {
-        lhs = rhs;
-      }
-      tint_msl_is_first_1 = false;
-
-      lhs = 2.400000095f;
-      other = 0.0f;
+  EXPECT_EQ(gen.result(), R"(  while (true) {
+    float lhs = 2.400000095f;
+    float other = 0.0f;
+    {
+      lhs = rhs;
     }
   }
 )");