writer/hlsl: 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.

Also fix random indenting of intrinsic functions.

Fixed: tint:744
Fixed: tint:818
Change-Id: I06994dbc724bc646e0435a1035b00760eaf5f5ab
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/51784
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: David Neto <dneto@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/writer/hlsl/generator_impl.cc

@@ -34,6 +34,7 @@
 #include "src/sem/variable.h"
 #include "src/transform/calculate_array_length.h"
 #include "src/transform/decompose_storage_access.h"
+#include "src/utils/scoped_assignment.h"
 #include "src/writer/append_vector.h"
 #include "src/writer/float_to_string.h"
 
@@ -422,20 +423,14 @@ bool GeneratorImpl::EmitBinary(std::ostream& pre,
 
 bool GeneratorImpl::EmitBlock(std::ostream& out,
                               const ast::BlockStatement* stmt) {
-  out << "{" << std::endl;
-  increment_indent();
-
-  for (auto* s : *stmt) {
-    if (!EmitStatement(out, s)) {
-      return false;
+  return EmitBlockBraces(out, [&] {
+    for (auto* s : *stmt) {
+      if (!EmitStatement(out, s)) {
+        return false;
+      }
     }
-  }
-
-  decrement_indent();
-  make_indent(out);
-  out << "}";
-
-  return true;
+    return true;
+  });
 }
 
 bool GeneratorImpl::EmitBlockAndNewline(std::ostream& out,
@@ -614,7 +609,6 @@ bool GeneratorImpl::EmitCall(std::ostream& pre,
       return false;
     }
 
-    make_indent(out);
     out << name << "(";
 
     bool first = true;
@@ -1338,6 +1332,9 @@ bool GeneratorImpl::EmitTypeConstructor(std::ostream& pre,
 }
 
 bool GeneratorImpl::EmitContinue(std::ostream& out, ast::ContinueStatement*) {
+  if (!emit_continuing_(out)) {
+    return false;
+  }
   make_indent(out);
   out << "continue;" << std::endl;
   return true;
@@ -2192,100 +2189,30 @@ bool GeneratorImpl::EmitZeroValue(std::ostream& out, const sem::Type* type) {
 }
 
 bool GeneratorImpl::EmitLoop(std::ostream& out, ast::LoopStatement* stmt) {
-  loop_emission_counter_++;
+  make_indent(out);
 
-  std::string guard = generate_name("tint_hlsl_is_first_" +
-                                    std::to_string(loop_emission_counter_));
+  auto emit_continuing = [this, stmt](std::ostream& o) {
+    if (stmt->has_continuing()) {
+      make_indent(o);
+      if (!EmitBlock(o, stmt->continuing())) {
+        return false;
+      }
+      o << std::endl;
+    }
+    return true;
+  };
 
-  if (stmt->has_continuing()) {
-    make_indent(out);
-
-    // Continuing variables get their own scope.
-    out << "{" << std::endl;
-    increment_indent();
-
-    make_indent(out);
-    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* v = s->As<ast::VariableDeclStatement>()) {
-        if (!EmitVariable(out, v->variable(), true)) {
-          return false;
-        }
+  TINT_SCOPED_ASSIGNMENT(emit_continuing_, emit_continuing);
+  bool ok = EmitBlockBraces(out, "while (true)", [&] {
+    for (auto* s : stmt->body()->statements()) {
+      if (!EmitStatement(out, s)) {
+        return false;
       }
     }
-  }
-
-  make_indent(out);
-  out << "for(;;) {" << std::endl;
-  increment_indent();
-
-  if (stmt->has_continuing()) {
-    make_indent(out);
-    out << "if (!" << guard << ") ";
-
-    if (!EmitBlockAndNewline(out, stmt->continuing())) {
-      return false;
-    }
-
-    make_indent(out);
-    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.
-    if (auto* decl = s->As<ast::VariableDeclStatement>()) {
-      if (stmt->has_continuing()) {
-        make_indent(out);
-
-        auto* var = decl->variable();
-
-        std::ostringstream pre;
-        std::ostringstream constructor_out;
-        if (var->constructor() != nullptr) {
-          if (!EmitExpression(pre, constructor_out, var->constructor())) {
-            return false;
-          }
-        }
-        out << pre.str();
-
-        out << builder_.Symbols().NameFor(var->symbol()) << " = ";
-        if (var->constructor() != nullptr) {
-          out << constructor_out.str();
-        } else {
-          auto* type = builder_.Sem().Get(var)->Type()->UnwrapRef();
-          if (!EmitZeroValue(out, type)) {
-            return false;
-          }
-        }
-        out << ";" << std::endl;
-        continue;
-      }
-    }
-
-    if (!EmitStatement(out, s)) {
-      return false;
-    }
-  }
-
-  decrement_indent();
-  make_indent(out);
-  out << "}" << std::endl;
-
-  // Close the scope for any continuing variables.
-  if (stmt->has_continuing()) {
-    decrement_indent();
-    make_indent(out);
-    out << "}" << std::endl;
-  }
-
-  return true;
+    return emit_continuing(out);
+  });
+  out << std::endl;
+  return ok;
 }
 
 bool GeneratorImpl::EmitMemberAccessor(std::ostream& pre,
@@ -2379,7 +2306,7 @@ bool GeneratorImpl::EmitStatement(std::ostream& out, ast::Statement* stmt) {
     return EmitSwitch(out, s);
   }
   if (auto* v = stmt->As<ast::VariableDeclStatement>()) {
-    return EmitVariable(out, v->variable(), false);
+    return EmitVariable(out, v->variable());
   }
 
   diagnostics_.add_error("unknown statement type: " + builder_.str(stmt));
@@ -2662,9 +2589,7 @@ bool GeneratorImpl::EmitUnaryOp(std::ostream& pre,
   return true;
 }
 
-bool GeneratorImpl::EmitVariable(std::ostream& out,
-                                 ast::Variable* var,
-                                 bool skip_constructor) {
+bool GeneratorImpl::EmitVariable(std::ostream& out, ast::Variable* var) {
   make_indent(out);
 
   auto* sem = builder_.Sem().Get(var);
@@ -2677,19 +2602,17 @@ bool GeneratorImpl::EmitVariable(std::ostream& out,
   }
 
   std::ostringstream constructor_out;
-  if (!skip_constructor) {
-    constructor_out << " = ";
+  constructor_out << " = ";
 
-    if (var->constructor()) {
-      std::ostringstream pre;
-      if (!EmitExpression(pre, constructor_out, var->constructor())) {
-        return false;
-      }
-      out << pre.str();
-    } else {
-      if (!EmitZeroValue(constructor_out, type)) {
-        return false;
-      }
+  if (var->constructor()) {
+    std::ostringstream pre;
+    if (!EmitExpression(pre, constructor_out, var->constructor())) {
+      return false;
+    }
+    out << pre.str();
+  } else {
+    if (!EmitZeroValue(constructor_out, type)) {
+      return false;
     }
   }
 
@@ -2789,6 +2712,23 @@ std::string GeneratorImpl::get_buffer_name(ast::Expression* expr) {
   return "";
 }
 
+template <typename F>
+bool GeneratorImpl::EmitBlockBraces(std::ostream& out,
+                                    const std::string& prefix,
+                                    F&& cb) {
+  out << prefix << (prefix.empty() ? "{" : " {") << std::endl;
+  increment_indent();
+
+  if (!cb()) {
+    return false;
+  }
+
+  decrement_indent();
+  make_indent(out);
+  out << "}";
+  return true;
+}
+
 }  // namespace hlsl
 }  // namespace writer
 }  // namespace tint

src/writer/hlsl/generator_impl.h

@@ -18,6 +18,7 @@
 #include <string>
 #include <unordered_map>
 #include <unordered_set>
+#include <utility>
 
 #include "src/ast/assignment_statement.h"
 #include "src/ast/bitcast_expression.h"
@@ -319,11 +320,8 @@ class GeneratorImpl : public TextGenerator {
   /// Handles generating a variable
   /// @param out the output stream
   /// @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(std::ostream& out,
-                    ast::Variable* var,
-                    bool skip_constructor);
+  bool EmitVariable(std::ostream& out, ast::Variable* var);
   /// Handles generating a program scope constant variable
   /// @param out the output stream
   /// @param var the variable to emit
@@ -387,10 +385,31 @@ class GeneratorImpl : public TextGenerator {
     return builder_.TypeOf(type);
   }
 
+  /// Emits `prefix`, followed by an opening brace `{`, then calls `cb` to emit
+  /// the block body, then finally emits the closing brace `}`.
+  /// @param out the output stream
+  /// @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(std::ostream& out, const std::string& prefix, F&& cb);
+
+  /// Emits an opening brace `{`, then calls `cb` to emit the block body, then
+  /// finally emits the closing brace `}`.
+  /// @param out the output stream
+  /// @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(std::ostream& out, F&& cb) {
+    return EmitBlockBraces(out, "", std::forward<F>(cb));
+  }
+
   ProgramBuilder builder_;
   Symbol current_ep_sym_;
   bool generating_entry_point_ = false;
-  uint32_t loop_emission_counter_ = 0;
+  std::function<bool(std::ostream& out)> emit_continuing_;
   ScopeStack<const sem::Variable*> global_variables_;
   std::unordered_map<Symbol, EntryPointData> ep_sym_to_in_data_;
   std::unordered_map<Symbol, EntryPointData> ep_sym_to_out_data_;

src/writer/hlsl/generator_impl_continue_test.cc

@@ -22,15 +22,18 @@ namespace {
 using HlslGeneratorImplTest_Continue = TestHelper;
 
 TEST_F(HlslGeneratorImplTest_Continue, 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(out, c)) << gen.error();
-  EXPECT_EQ(result(), "  continue;\n");
+  ASSERT_TRUE(gen.EmitStatement(out, loop)) << gen.error();
+  EXPECT_EQ(result(), R"(  while (true) {
+    continue;
+  }
+)");
 }
 
 }  // namespace

src/writer/hlsl/generator_impl_intrinsic_test.cc

@@ -268,7 +268,7 @@ TEST_F(HlslGeneratorImplTest_Intrinsic, Intrinsic_Call) {
 
   gen.increment_indent();
   ASSERT_TRUE(gen.EmitExpression(pre, out, call)) << gen.error();
-  EXPECT_EQ(result(), "  dot(param1, param2)");
+  EXPECT_EQ(result(), "dot(param1, param2)");
 }
 
 TEST_F(HlslGeneratorImplTest_Intrinsic, Pack4x8Snorm) {

src/writer/hlsl/generator_impl_loop_test.cc

@@ -34,7 +34,7 @@ TEST_F(HlslGeneratorImplTest_Loop, Emit_Loop) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(out, l)) << gen.error();
-  EXPECT_EQ(result(), R"(  for(;;) {
+  EXPECT_EQ(result(), R"(  while (true) {
     discard;
   }
 )");
@@ -52,15 +52,10 @@ TEST_F(HlslGeneratorImplTest_Loop, Emit_LoopWithContinuing) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(out, l)) << gen.error();
-  EXPECT_EQ(result(), R"(  {
-    bool tint_hlsl_is_first_1 = true;
-    for(;;) {
-      if (!tint_hlsl_is_first_1) {
-        return;
-      }
-      tint_hlsl_is_first_1 = false;
-
-      discard;
+  EXPECT_EQ(result(), R"(  while (true) {
+    discard;
+    {
+      return;
     }
   }
 )");
@@ -89,26 +84,16 @@ TEST_F(HlslGeneratorImplTest_Loop, Emit_LoopNestedWithContinuing) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(out, outer)) << gen.error();
-  EXPECT_EQ(result(), R"(  {
-    bool tint_hlsl_is_first_1 = true;
-    for(;;) {
-      if (!tint_hlsl_is_first_1) {
-        lhs = rhs;
-      }
-      tint_hlsl_is_first_1 = false;
-
+  EXPECT_EQ(result(), R"(  while (true) {
+    while (true) {
+      discard;
       {
-        bool tint_hlsl_is_first_2 = true;
-        for(;;) {
-          if (!tint_hlsl_is_first_2) {
-            return;
-          }
-          tint_hlsl_is_first_2 = false;
-
-          discard;
-        }
+        return;
       }
     }
+    {
+      lhs = rhs;
+    }
   }
 )");
 }
@@ -153,18 +138,11 @@ TEST_F(HlslGeneratorImplTest_Loop, Emit_LoopWithVarUsedInContinuing) {
   gen.increment_indent();
 
   ASSERT_TRUE(gen.EmitStatement(out, outer)) << gen.error();
-  EXPECT_EQ(result(), R"(  {
-    bool tint_hlsl_is_first_1 = true;
-    float lhs;
-    float other;
-    for(;;) {
-      if (!tint_hlsl_is_first_1) {
-        lhs = rhs;
-      }
-      tint_hlsl_is_first_1 = false;
-
-      lhs = 2.400000095f;
-      other = 0.0f;
+  EXPECT_EQ(result(), R"(  while (true) {
+    float lhs = 2.400000095f;
+    float other = 0.0f;
+    {
+      lhs = rhs;
     }
   }
 )");