[msl-writer][hlsl-writer] Pull loop variables out with continuing.

If there is a continuing block we pull the variables declared in the
loop up into the scope outside the loop. This allows those variables to
be used in the continuing block.

We pull out all variables instead of detecting ones which are only used
in continuing as that's easier and still correct.

Bug: tint:187, tint:186
Change-Id: I1de0e36111a236ff04a323cf9777bc79e67afa77
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27620
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: Sarah Mashayekhi <sarahmashay@google.com>
Reviewed-by: David Neto <dneto@google.com>

src/writer/hlsl/generator_impl.cc

@@ -1470,6 +1470,19 @@ bool GeneratorImpl::EmitLoop(std::ostream& out, ast::LoopStatement* stmt) {
 
     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 (const auto& s : *(stmt->body())) {
+      if (!s->IsVariableDecl()) {
+        continue;
+      }
+      if (!EmitVariable(out, s->AsVariableDecl()->variable(), true)) {
+        return false;
+      }
+    }
   }
 
   make_indent(out);
@@ -1490,6 +1503,26 @@ bool GeneratorImpl::EmitLoop(std::ostream& out, ast::LoopStatement* stmt) {
   }
 
   for (const 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 (s->IsVariableDecl() && stmt->has_continuing()) {
+      make_indent(out);
+
+      auto* var = s->AsVariableDecl()->variable();
+      out << var->name() << " = ";
+      if (var->constructor() != nullptr) {
+        if (!EmitExpression(out, var->constructor())) {
+          return false;
+        }
+      } else {
+        if (!EmitZeroValue(out, var->type())) {
+          return false;
+        }
+      }
+      out << ";" << std::endl;
+      continue;
+    }
+
     if (!EmitStatement(out, s.get())) {
       return false;
     }
@@ -1831,7 +1864,7 @@ bool GeneratorImpl::EmitStatement(std::ostream& out, ast::Statement* stmt) {
     return EmitSwitch(out, stmt->AsSwitch());
   }
   if (stmt->IsVariableDecl()) {
-    return EmitVariable(out, stmt->AsVariableDecl()->variable());
+    return EmitVariable(out, stmt->AsVariableDecl()->variable(), false);
   }
 
   error_ = "unknown statement type: " + stmt->str();
@@ -1982,7 +2015,9 @@ bool GeneratorImpl::EmitUnaryOp(std::ostream& out,
   return true;
 }
 
-bool GeneratorImpl::EmitVariable(std::ostream& out, ast::Variable* var) {
+bool GeneratorImpl::EmitVariable(std::ostream& out,
+                                 ast::Variable* var,
+                                 bool skip_constructor) {
   make_indent(out);
 
   // TODO(dsinclair): Handle variable decorations
@@ -2001,7 +2036,7 @@ bool GeneratorImpl::EmitVariable(std::ostream& out, ast::Variable* var) {
     out << " " << var->name();
   }
 
-  if (var->constructor() != nullptr) {
+  if (!skip_constructor && var->constructor() != nullptr) {
     out << " = ";
     if (!EmitExpression(out, var->constructor())) {
       return false;

src/writer/hlsl/generator_impl.h

@@ -255,8 +255,11 @@ class GeneratorImpl {
   /// 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 EmitVariable(std::ostream& out,
+                    ast::Variable* var,
+                    bool skip_constructor);
   /// Handles generating a program scope constant variable
   /// @param out the output stream
   /// @param var the variable to emit

src/writer/hlsl/generator_impl_loop_test.cc

@@ -122,9 +122,28 @@ TEST_F(HlslGeneratorImplTest_Loop, Emit_LoopNestedWithContinuing) {
 )");
 }
 
-// TODO(dsinclair): Handle pulling declared variables up and out of the for() if
+TEST_F(HlslGeneratorImplTest_Loop, Emit_LoopWithVarUsedInContinuing) {
-// there is a continuing block.
+  // loop {
-TEST_F(HlslGeneratorImplTest_Loop, DISABLED_Emit_LoopWithVarUsedInContinuing) {
+  //   var lhs : f32 = 2.4;
+  //   var other : f32;
+  //   continuing {
+  //     lhs = rhs
+  //   }
+  // }
+  //
+  // ->
+  // {
+  //   float lhs;
+  //   float other;
+  //   for (;;) {
+  //     if (continuing) {
+  //       lhs = rhs;
+  //     }
+  //     lhs = 2.4f;
+  //     other = 0.0f;
+  //   }
+  // }
+
   ast::type::F32Type f32;
 
   auto var = std::make_unique<ast::Variable>(
@@ -150,8 +169,9 @@ TEST_F(HlslGeneratorImplTest_Loop, DISABLED_Emit_LoopWithVarUsedInContinuing) {
 
   ASSERT_TRUE(gen().EmitStatement(out(), &outer)) << gen().error();
   EXPECT_EQ(result(), R"(  {
-    float lhs;
     bool tint_hlsl_is_first_1 = true;
+    float lhs;
+    float other;
     for(;;) {
       if (!tint_hlsl_is_first_1) {
         lhs = rhs;
@@ -159,7 +179,7 @@ TEST_F(HlslGeneratorImplTest_Loop, DISABLED_Emit_LoopWithVarUsedInContinuing) {
       tint_hlsl_is_first_1 = false;
 
       lhs = 2.40000010f;
-      float other;
+      other = 0.0f;
     }
   }
 )");

src/writer/msl/generator_impl.cc

@@ -1462,6 +1462,19 @@ bool GeneratorImpl::EmitLoop(ast::LoopStatement* stmt) {
 
     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 (const auto& s : *(stmt->body())) {
+      if (!s->IsVariableDecl()) {
+        continue;
+      }
+      if (!EmitVariable(s->AsVariableDecl()->variable(), true)) {
+        return false;
+      }
+    }
   }
 
   make_indent();
@@ -1482,6 +1495,26 @@ bool GeneratorImpl::EmitLoop(ast::LoopStatement* stmt) {
   }
 
   for (const 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 (s->IsVariableDecl() && stmt->has_continuing()) {
+      make_indent();
+
+      auto* var = s->AsVariableDecl()->variable();
+      out_ << var->name() << " = ";
+      if (var->constructor() != nullptr) {
+        if (!EmitExpression(var->constructor())) {
+          return false;
+        }
+      } else {
+        if (!EmitZeroValue(var->type())) {
+          return false;
+        }
+      }
+      out_ << ";" << std::endl;
+      continue;
+    }
+
     if (!EmitStatement(s.get())) {
       return false;
     }
@@ -1652,7 +1685,7 @@ bool GeneratorImpl::EmitStatement(ast::Statement* stmt) {
     return EmitSwitch(stmt->AsSwitch());
   }
   if (stmt->IsVariableDecl()) {
-    return EmitVariable(stmt->AsVariableDecl()->variable());
+    return EmitVariable(stmt->AsVariableDecl()->variable(), false);
   }
 
   error_ = "unknown statement type: " + stmt->str();
@@ -1815,7 +1848,7 @@ bool GeneratorImpl::EmitUnaryOp(ast::UnaryOpExpression* expr) {
   return true;
 }
 
-bool GeneratorImpl::EmitVariable(ast::Variable* var) {
+bool GeneratorImpl::EmitVariable(ast::Variable* var, bool skip_constructor) {
   make_indent();
 
   // TODO(dsinclair): Handle variable decorations
@@ -1834,7 +1867,7 @@ bool GeneratorImpl::EmitVariable(ast::Variable* var) {
     out_ << " " << var->name();
   }
 
-  if (var->constructor() != nullptr) {
+  if (!skip_constructor && var->constructor() != nullptr) {
     out_ << " = ";
     if (!EmitExpression(var->constructor())) {
       return false;

src/writer/msl/generator_impl.h

@@ -200,8 +200,9 @@ 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(ast::Variable* var);
+  bool EmitVariable(ast::Variable* var, bool skip_constructor);
   /// Handles generating a program scope constant variable
   /// @param var the variable to emit
   /// @returns true if the variable was emitted

src/writer/msl/generator_impl_loop_test.cc

@@ -132,9 +132,28 @@ TEST_F(MslGeneratorImplTest, Emit_LoopNestedWithContinuing) {
 )");
 }
 
-// TODO(dsinclair): Handle pulling declared variables up and out of the for() if
+TEST_F(MslGeneratorImplTest, Emit_LoopWithVarUsedInContinuing) {
-// there is a continuing block.
+  // loop {
-TEST_F(MslGeneratorImplTest, DISABLED_Emit_LoopWithVarUsedInContinuing) {
+  //   var lhs : f32 = 2.4;
+  //   var other : f32;
+  //   continuing {
+  //     lhs = rhs
+  //   }
+  // }
+  //
+  // ->
+  // {
+  //   float lhs;
+  //   float other;
+  //   for (;;) {
+  //     if (continuing) {
+  //       lhs = rhs;
+  //     }
+  //     lhs = 2.4f;
+  //     other = 0.0f;
+  //   }
+  // }
+
   ast::type::F32Type f32;
 
   auto var = std::make_unique<ast::Variable>(
@@ -155,16 +174,17 @@ TEST_F(MslGeneratorImplTest, DISABLED_Emit_LoopWithVarUsedInContinuing) {
   continuing->append(std::make_unique<ast::AssignmentStatement>(
       std::move(lhs), std::move(rhs)));
 
-  ast::LoopStatement outer(std::move(body), std::move(continuing));
-
   ast::Module m;
   GeneratorImpl g(&m);
   g.increment_indent();
 
+  ast::LoopStatement outer(std::move(body), std::move(continuing));
+
   ASSERT_TRUE(g.EmitStatement(&outer)) << g.error();
   EXPECT_EQ(g.result(), R"(  {
-    float lhs;
     bool tint_msl_is_first_1 = true;
+    float lhs;
+    float other;
     for(;;) {
       if (!tint_msl_is_first_1) {
         lhs = rhs;
@@ -172,7 +192,7 @@ TEST_F(MslGeneratorImplTest, DISABLED_Emit_LoopWithVarUsedInContinuing) {
       tint_msl_is_first_1 = false;
 
       lhs = 2.40000010f;
-      float other;
+      other = 0.0f;
     }
   }
 )");