Split back out two transforms previously combined into PromoteSideEffectsToDeclTest

That is, bring back both VarForDynamicIndex and
PromoteInitializersToConstVar. This is not a complete revert, though:
*  VarForDynamicIndex no longer depends on ForLoopToLoop
* Both can cope with hoisting from "else if"
* More unit tests were added in the interim

Delete PromoteSideEffectsToDecl for now. This may be brought back to
handle ensuring order of evaluation.

Bug: tint:1300
Change-Id: I8bbae46377ec4603cc02c1eb3f0661a8461a19fa
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/79940
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>

src/BUILD.gn

@@ -470,8 +470,8 @@ libtint_source_set("libtint_core_all_src") {
     "transform/num_workgroups_from_uniform.h",
     "transform/pad_array_elements.cc",
     "transform/pad_array_elements.h",
-    "transform/promote_side_effects_to_decl.cc",
+    "transform/promote_initializers_to_const_var.cc",
-    "transform/promote_side_effects_to_decl.h",
+    "transform/promote_initializers_to_const_var.h",
     "transform/remove_phonies.cc",
     "transform/remove_phonies.h",
     "transform/remove_unreachable_statements.cc",
@@ -488,6 +488,10 @@ libtint_source_set("libtint_core_all_src") {
     "transform/transform.h",
     "transform/unshadow.cc",
     "transform/unshadow.h",
+    "transform/utils/hoist_to_decl_before.cc",
+    "transform/utils/hoist_to_decl_before.h",
+    "transform/var_for_dynamic_index.cc",
+    "transform/var_for_dynamic_index.h",
     "transform/vectorize_scalar_matrix_constructors.cc",
     "transform/vectorize_scalar_matrix_constructors.h",
     "transform/vertex_pulling.cc",

src/CMakeLists.txt

@@ -343,8 +343,8 @@ set(TINT_LIB_SRCS
   transform/num_workgroups_from_uniform.h
   transform/pad_array_elements.cc
   transform/pad_array_elements.h
-  transform/promote_side_effects_to_decl.cc
+  transform/promote_initializers_to_const_var.cc
-  transform/promote_side_effects_to_decl.h
+  transform/promote_initializers_to_const_var.h
   transform/remove_phonies.cc
   transform/remove_phonies.h
   transform/remove_unreachable_statements.cc
@@ -363,12 +363,16 @@ set(TINT_LIB_SRCS
   transform/unshadow.h
   transform/vectorize_scalar_matrix_constructors.cc
   transform/vectorize_scalar_matrix_constructors.h
+  transform/var_for_dynamic_index.cc
+  transform/var_for_dynamic_index.h
   transform/vertex_pulling.cc
   transform/vertex_pulling.h
   transform/wrap_arrays_in_structs.cc
   transform/wrap_arrays_in_structs.h
   transform/zero_init_workgroup_memory.cc
   transform/zero_init_workgroup_memory.h
+  transform/utils/hoist_to_decl_before.cc
+  transform/utils/hoist_to_decl_before.h
   sem/bool_type.cc
   sem/bool_type.h
   sem/depth_texture_type.cc
@@ -1011,7 +1015,7 @@ if(TINT_BUILD_TESTS)
       transform/multiplanar_external_texture_test.cc
       transform/num_workgroups_from_uniform_test.cc
       transform/pad_array_elements_test.cc
-      transform/promote_side_effects_to_decl_test.cc
+      transform/promote_initializers_to_const_var_test.cc
       transform/remove_phonies_test.cc
       transform/remove_unreachable_statements_test.cc
       transform/renamer_test.cc
@@ -1020,10 +1024,12 @@ if(TINT_BUILD_TESTS)
       transform/single_entry_point_test.cc
       transform/test_helper.h
       transform/unshadow_test.cc
+      transform/var_for_dynamic_index_test.cc
       transform/vectorize_scalar_matrix_constructors_test.cc
       transform/vertex_pulling_test.cc
       transform/wrap_arrays_in_structs_test.cc
       transform/zero_init_workgroup_memory_test.cc
+      transform/utils/hoist_to_decl_before_test.cc
     )
   endif()
 

src/transform/glsl.cc

@@ -28,7 +28,7 @@
 #include "src/transform/loop_to_for_loop.h"
 #include "src/transform/manager.h"
 #include "src/transform/pad_array_elements.h"
-#include "src/transform/promote_side_effects_to_decl.h"
+#include "src/transform/promote_initializers_to_const_var.h"
 #include "src/transform/remove_phonies.h"
 #include "src/transform/renamer.h"
 #include "src/transform/simplify_pointers.h"
@@ -89,10 +89,7 @@ Output Glsl::Run(const Program* in, const DataMap& inputs) const {
     data.Add<BindingRemapper::Remappings>(bp, ac, /* mayCollide */ true);
   }
   manager.Add<ExternalTextureTransform>();
-
+  manager.Add<PromoteInitializersToConstVar>();
-  data.Add<PromoteSideEffectsToDecl::Config>(
-      /* type_ctor_to_let */ true, /* dynamic_index_to_var */ false);
-  manager.Add<PromoteSideEffectsToDecl>();
 
   manager.Add<PadArrayElements>();
   manager.Add<AddEmptyEntryPoint>();

src/transform/promote_initializers_to_const_var.cc

@@ -0,0 +1,83 @@
+// 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/transform/promote_initializers_to_const_var.h"
+#include "src/program_builder.h"
+#include "src/sem/call.h"
+#include "src/sem/statement.h"
+#include "src/sem/type_constructor.h"
+#include "src/transform/utils/hoist_to_decl_before.h"
+
+TINT_INSTANTIATE_TYPEINFO(tint::transform::PromoteInitializersToConstVar);
+
+namespace tint::transform {
+
+PromoteInitializersToConstVar::PromoteInitializersToConstVar() = default;
+
+PromoteInitializersToConstVar::~PromoteInitializersToConstVar() = default;
+
+void PromoteInitializersToConstVar::Run(CloneContext& ctx,
+                                        const DataMap&,
+                                        DataMap&) const {
+  HoistToDeclBefore hoist_to_decl_before(ctx);
+
+  // Hoists array and structure initializers to a constant variable, declared
+  // just before the statement of usage.
+  auto type_ctor_to_let = [&](const ast::CallExpression* expr) {
+    auto* ctor = ctx.src->Sem().Get(expr);
+    if (!ctor->Target()->Is<sem::TypeConstructor>()) {
+      return true;
+    }
+    auto* sem_stmt = ctor->Stmt();
+    if (!sem_stmt) {
+      // Expression is outside of a statement. This usually means the
+      // expression is part of a global (module-scope) constant declaration.
+      // These must be constexpr, and so cannot contain the type of
+      // expressions that must be sanitized.
+      return true;
+    }
+
+    auto* stmt = sem_stmt->Declaration();
+
+    if (auto* src_var_decl = stmt->As<ast::VariableDeclStatement>()) {
+      if (src_var_decl->variable->constructor == expr) {
+        // This statement is just a variable declaration with the
+        // initializer as the constructor value. This is what we're
+        // attempting to transform to, and so ignore.
+        return true;
+      }
+    }
+
+    auto* src_ty = ctor->Type();
+    if (!src_ty->IsAnyOf<sem::Array, sem::Struct>()) {
+      // We only care about array and struct initializers
+      return true;
+    }
+
+    return hoist_to_decl_before.Add(ctor, expr, true);
+  };
+
+  for (auto* node : ctx.src->ASTNodes().Objects()) {
+    if (auto* call_expr = node->As<ast::CallExpression>()) {
+      if (!type_ctor_to_let(call_expr)) {
+        return;
+      }
+    }
+  }
+
+  hoist_to_decl_before.Apply();
+  ctx.Clone();
+}
+
+}  // namespace tint::transform

src/transform/promote_initializers_to_const_var.h

@@ -0,0 +1,48 @@
+// 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_TRANSFORM_PROMOTE_INITIALIZERS_TO_CONST_VAR_H_
+#define SRC_TRANSFORM_PROMOTE_INITIALIZERS_TO_CONST_VAR_H_
+
+#include "src/transform/transform.h"
+
+namespace tint::transform {
+
+/// A transform that hoists the array and structure initializers to a constant
+/// variable, declared just before the statement of usage.
+/// @see crbug.com/tint/406
+class PromoteInitializersToConstVar
+    : public Castable<PromoteInitializersToConstVar, Transform> {
+ public:
+  /// Constructor
+  PromoteInitializersToConstVar();
+
+  /// Destructor
+  ~PromoteInitializersToConstVar() override;
+
+ protected:
+  /// 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_TRANSFORM_PROMOTE_INITIALIZERS_TO_CONST_VAR_H_

src/transform/promote_initializers_to_const_var_test.cc

@@ -0,0 +1,491 @@
+// Copyright 2021 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/transform/promote_initializers_to_const_var.h"
+
+#include "src/transform/test_helper.h"
+
+namespace tint {
+namespace transform {
+namespace {
+
+using PromoteInitializersToConstVarTest = TransformTest;
+
+TEST_F(PromoteInitializersToConstVarTest, EmptyModule) {
+  auto* src = "";
+  auto* expect = "";
+
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, BasicArray) {
+  auto* src = R"(
+fn f() {
+  var f0 = 1.0;
+  var f1 = 2.0;
+  var f2 = 3.0;
+  var f3 = 4.0;
+  var i = array<f32, 4u>(f0, f1, f2, f3)[2];
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var f0 = 1.0;
+  var f1 = 2.0;
+  var f2 = 3.0;
+  var f3 = 4.0;
+  let tint_symbol = array<f32, 4u>(f0, f1, f2, f3);
+  var i = tint_symbol[2];
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, BasicStruct) {
+  auto* src = R"(
+struct S {
+  a : i32;
+  b : f32;
+  c : vec3<f32>;
+};
+
+fn f() {
+  var x = S(1, 2.0, vec3<f32>()).b;
+}
+)";
+
+  auto* expect = R"(
+struct S {
+  a : i32;
+  b : f32;
+  c : vec3<f32>;
+}
+
+fn f() {
+  let tint_symbol = S(1, 2.0, vec3<f32>());
+  var x = tint_symbol.b;
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, ArrayInForLoopInit) {
+  auto* src = R"(
+fn f() {
+  var insert_after = 1;
+  for(var i = array<f32, 4u>(0.0, 1.0, 2.0, 3.0)[2]; ; ) {
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var insert_after = 1;
+  let tint_symbol = array<f32, 4u>(0.0, 1.0, 2.0, 3.0);
+  for(var i = tint_symbol[2]; ; ) {
+    break;
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, StructInForLoopInit) {
+  auto* src = R"(
+struct S {
+  a : i32;
+  b : f32;
+  c : vec3<f32>;
+};
+
+fn f() {
+  var insert_after = 1;
+  for(var x = S(1, 2.0, vec3<f32>()).b; ; ) {
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+struct S {
+  a : i32;
+  b : f32;
+  c : vec3<f32>;
+}
+
+fn f() {
+  var insert_after = 1;
+  let tint_symbol = S(1, 2.0, vec3<f32>());
+  for(var x = tint_symbol.b; ; ) {
+    break;
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, ArrayInForLoopCond) {
+  auto* src = R"(
+fn f() {
+  var f = 1.0;
+  for(; f == array<f32, 1u>(f)[0]; f = f + 1.0) {
+    var marker = 1;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var f = 1.0;
+  loop {
+    let tint_symbol = array<f32, 1u>(f);
+    if (!((f == tint_symbol[0]))) {
+      break;
+    }
+    {
+      var marker = 1;
+    }
+
+    continuing {
+      f = (f + 1.0);
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, ArrayInForLoopCont) {
+  auto* src = R"(
+fn f() {
+  var f = 0.0;
+  for(; f < 10.0; f = f + array<f32, 1u>(1.0)[0]) {
+    var marker = 1;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var f = 0.0;
+  loop {
+    if (!((f < 10.0))) {
+      break;
+    }
+    {
+      var marker = 1;
+    }
+
+    continuing {
+      let tint_symbol = array<f32, 1u>(1.0);
+      f = (f + tint_symbol[0]);
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, ArrayInForLoopInitCondCont) {
+  auto* src = R"(
+fn f() {
+  for(var f = array<f32, 1u>(0.0)[0];
+      f < array<f32, 1u>(1.0)[0];
+      f = f + array<f32, 1u>(2.0)[0]) {
+    var marker = 1;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  let tint_symbol = array<f32, 1u>(0.0);
+  {
+    var f = tint_symbol[0];
+    loop {
+      let tint_symbol_1 = array<f32, 1u>(1.0);
+      if (!((f < tint_symbol_1[0]))) {
+        break;
+      }
+      {
+        var marker = 1;
+      }
+
+      continuing {
+        let tint_symbol_2 = array<f32, 1u>(2.0);
+        f = (f + tint_symbol_2[0]);
+      }
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, ArrayInElseIf) {
+  auto* src = R"(
+fn f() {
+  var f = 1.0;
+  if (true) {
+    var marker = 0;
+  } else if (f == array<f32, 2u>(f, f)[0]) {
+    var marker = 1;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var f = 1.0;
+  if (true) {
+    var marker = 0;
+  } else {
+    let tint_symbol = array<f32, 2u>(f, f);
+    if ((f == tint_symbol[0])) {
+      var marker = 1;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, ArrayInElseIfChain) {
+  auto* src = R"(
+fn f() {
+  var f = 1.0;
+  if (true) {
+    var marker = 0;
+  } else if (true) {
+    var marker = 1;
+  } else if (f == array<f32, 2u>(f, f)[0]) {
+    var marker = 2;
+  } else if (f == array<f32, 2u>(f, f)[1]) {
+    var marker = 3;
+  } else if (true) {
+    var marker = 4;
+  } else {
+    var marker = 5;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var f = 1.0;
+  if (true) {
+    var marker = 0;
+  } else if (true) {
+    var marker = 1;
+  } else {
+    let tint_symbol = array<f32, 2u>(f, f);
+    if ((f == tint_symbol[0])) {
+      var marker = 2;
+    } else {
+      let tint_symbol_1 = array<f32, 2u>(f, f);
+      if ((f == tint_symbol_1[1])) {
+        var marker = 3;
+      } else if (true) {
+        var marker = 4;
+      } else {
+        var marker = 5;
+      }
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, ArrayInArrayArray) {
+  auto* src = R"(
+fn f() {
+  var i = array<array<f32, 2u>, 2u>(array<f32, 2u>(1.0, 2.0), array<f32, 2u>(3.0, 4.0))[0][1];
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  let tint_symbol = array<f32, 2u>(1.0, 2.0);
+  let tint_symbol_1 = array<f32, 2u>(3.0, 4.0);
+  let tint_symbol_2 = array<array<f32, 2u>, 2u>(tint_symbol, tint_symbol_1);
+  var i = tint_symbol_2[0][1];
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, StructNested) {
+  auto* src = R"(
+struct S1 {
+  a : i32;
+};
+
+struct S2 {
+  a : i32;
+  b : S1;
+  c : i32;
+};
+
+struct S3 {
+  a : S2;
+};
+
+fn f() {
+  var x = S3(S2(1, S1(2), 3)).a.b.a;
+}
+)";
+
+  auto* expect = R"(
+struct S1 {
+  a : i32;
+}
+
+struct S2 {
+  a : i32;
+  b : S1;
+  c : i32;
+}
+
+struct S3 {
+  a : S2;
+}
+
+fn f() {
+  let tint_symbol = S1(2);
+  let tint_symbol_1 = S2(1, tint_symbol, 3);
+  let tint_symbol_2 = S3(tint_symbol_1);
+  var x = tint_symbol_2.a.b.a;
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, Mixed) {
+  auto* src = R"(
+struct S1 {
+  a : i32;
+};
+
+struct S2 {
+  a : array<S1, 3u>;
+};
+
+fn f() {
+  var x = S2(array<S1, 3u>(S1(1), S1(2), S1(3))).a[1].a;
+}
+)";
+
+  auto* expect = R"(
+struct S1 {
+  a : i32;
+}
+
+struct S2 {
+  a : array<S1, 3u>;
+}
+
+fn f() {
+  let tint_symbol = S1(1);
+  let tint_symbol_1 = S1(2);
+  let tint_symbol_2 = S1(3);
+  let tint_symbol_3 = array<S1, 3u>(tint_symbol, tint_symbol_1, tint_symbol_2);
+  let tint_symbol_4 = S2(tint_symbol_3);
+  var x = tint_symbol_4.a[1].a;
+}
+)";
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(PromoteInitializersToConstVarTest, NoChangeOnVarDecl) {
+  auto* src = R"(
+struct S {
+  a : i32;
+  b : f32;
+  c : i32;
+}
+
+fn f() {
+  var local_arr = array<f32, 4u>(0.0, 1.0, 2.0, 3.0);
+  var local_str = S(1, 2.0, 3);
+}
+
+let module_arr : array<f32, 4u> = array<f32, 4u>(0.0, 1.0, 2.0, 3.0);
+
+let module_str : S = S(1, 2.0, 3);
+)";
+
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<PromoteInitializersToConstVar>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+}  // namespace
+}  // namespace transform
+}  // namespace tint

src/transform/promote_side_effects_to_decl.h

@@ -1,75 +0,0 @@
-// Copyright 2021 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_TRANSFORM_PROMOTE_SIDE_EFFECTS_TO_DECL_H_
-#define SRC_TRANSFORM_PROMOTE_SIDE_EFFECTS_TO_DECL_H_
-
-#include "src/transform/transform.h"
-
-namespace tint {
-namespace transform {
-
-/// A transform that hoists expressions with side-effects to a variable
-/// declaration just before the statement of usage. This transform may also
-/// decompose for-loops into loops so that let declarations can be emitted
-/// before loop condition expressions and/or continuing statements. It may also
-/// similarly decompose 'else if's to 'else { if }'s for the same reason.
-/// @see crbug.com/tint/406
-class PromoteSideEffectsToDecl
-    : public Castable<PromoteSideEffectsToDecl, Transform> {
- public:
-  /// Constructor
-  PromoteSideEffectsToDecl();
-
-  /// Destructor
-  ~PromoteSideEffectsToDecl() override;
-
-  /// Configuration options for the transform.
-  struct Config : public Castable<Config, Data> {
-    /// Constructor
-    /// @param type_ctor_to_let whether to hoist type constructor expressions
-    /// to a let
-    /// @param dynamic_index_to_var whether to hoist dynamic indexed
-    /// expressions to a var
-    Config(bool type_ctor_to_let, bool dynamic_index_to_var);
-
-    /// Destructor
-    ~Config() override;
-
-    /// Whether to hoist type constructor expressions to a let
-    const bool type_ctor_to_let;
-
-    /// Whether to hoist dynamic indexed expressions to a var
-    const bool dynamic_index_to_var;
-  };
-
- protected:
-  /// 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;
-
- private:
-  class State;
-};
-
-}  // namespace transform
-}  // namespace tint
-
-#endif  // SRC_TRANSFORM_PROMOTE_SIDE_EFFECTS_TO_DECL_H_

src/transform/test_helper.h

@@ -28,6 +28,37 @@
 namespace tint {
 namespace transform {
 
+/// @param program the program to get an output WGSL string from
+/// @returns the output program as a WGSL string, or an error string if the
+/// program is not valid.
+inline std::string str(const Program& program) {
+  diag::Formatter::Style style;
+  style.print_newline_at_end = false;
+
+  if (!program.IsValid()) {
+    return diag::Formatter(style).format(program.Diagnostics());
+  }
+
+  writer::wgsl::Options options;
+  auto result = writer::wgsl::Generate(&program, options);
+  if (!result.success) {
+    return "WGSL writer failed:\n" + result.error;
+  }
+
+  auto res = result.wgsl;
+  if (res.empty()) {
+    return res;
+  }
+  // The WGSL sometimes has two trailing newlines. Strip them
+  while (res.back() == '\n') {
+    res.pop_back();
+  }
+  if (res.empty()) {
+    return res;
+  }
+  return "\n" + res + "\n";
+}
+
 /// Helper class for testing transforms
 template <typename BASE>
 class TransformTestBase : public BASE {
@@ -104,31 +135,7 @@ class TransformTestBase : public BASE {
   /// @returns the output program as a WGSL string, or an error string if the
   /// program is not valid.
   std::string str(const Output& output) {
-    diag::Formatter::Style style;
+    return transform::str(output.program);
-    style.print_newline_at_end = false;
-
-    if (!output.program.IsValid()) {
-      return diag::Formatter(style).format(output.program.Diagnostics());
-    }
-
-    writer::wgsl::Options options;
-    auto result = writer::wgsl::Generate(&output.program, options);
-    if (!result.success) {
-      return "WGSL writer failed:\n" + result.error;
-    }
-
-    auto res = result.wgsl;
-    if (res.empty()) {
-      return res;
-    }
-    // The WGSL sometimes has two trailing newlines. Strip them
-    while (res.back() == '\n') {
-      res.pop_back();
-    }
-    if (res.empty()) {
-      return res;
-    }
-    return "\n" + res + "\n";
   }
 
  private:

src/transform/utils/hoist_to_decl_before.cc

@@ -1,4 +1,4 @@
-// Copyright 2021 The Tint Authors.
+// 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.
@@ -12,46 +12,34 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include "src/transform/promote_side_effects_to_decl.h"
+#include "src/transform/utils/hoist_to_decl_before.h"
 
-#include <string>
 #include <unordered_map>
-#include <utility>
 
-#include "src/program_builder.h"
+#include "src/ast/variable_decl_statement.h"
 #include "src/sem/block_statement.h"
-#include "src/sem/call.h"
-#include "src/sem/expression.h"
 #include "src/sem/for_loop_statement.h"
 #include "src/sem/if_statement.h"
-#include "src/sem/statement.h"
-#include "src/sem/type_constructor.h"
 #include "src/utils/reverse.h"
 
-TINT_INSTANTIATE_TYPEINFO(tint::transform::PromoteSideEffectsToDecl);
+namespace tint::transform {
-TINT_INSTANTIATE_TYPEINFO(tint::transform::PromoteSideEffectsToDecl::Config);
 
-namespace tint {
+/// Private implementation of HoistToDeclBefore transform
-namespace transform {
+class HoistToDeclBefore::State {
-
-/// Private implementation of PromoteSideEffectsToDecl transform
-class PromoteSideEffectsToDecl::State {
- private:
   CloneContext& ctx;
-  const Config& cfg;
   ProgramBuilder& b;
 
   /// Holds information about a for-loop that needs to be decomposed into a
-  /// loop, so that declaration statements can be inserted before the condition
+  /// loop, so that declaration statements can be inserted before the
-  /// expression or continuing statement.
+  /// condition expression or continuing statement.
   struct LoopInfo {
     ast::StatementList cond_decls;
     ast::StatementList cont_decls;
   };
 
   /// Holds information about 'if's with 'else-if' statements that need to be
-  /// decomposed into 'if {else}' so that declaration statements can be inserted
+  /// decomposed into 'if {else}' so that declaration statements can be
-  /// before the condition expression.
+  /// inserted before the condition expression.
   struct IfInfo {
     /// Info for each else-if that needs decomposing
     struct ElseIfInfo {
@@ -63,14 +51,15 @@ class PromoteSideEffectsToDecl::State {
     std::unordered_map<const sem::ElseStatement*, ElseIfInfo> else_ifs;
   };
 
-  // For-loops that need to be decomposed to loops.
+  /// For-loops that need to be decomposed to loops.
   std::unordered_map<const sem::ForLoopStatement*, LoopInfo> loops;
 
-  /// If statements with 'else if's that need to be decomposed to 'else { if }'
+  /// If statements with 'else if's that need to be decomposed to 'else { if
+  /// }'
   std::unordered_map<const sem::IfStatement*, IfInfo> ifs;
 
   // Inserts `decl` before `sem_expr`, possibly marking a for-loop to be
-  // converted to a loop, or an else-if to an else { if }..
+  // converted to a loop, or an else-if to an else { if }.
   bool InsertBefore(const sem::Expression* sem_expr,
                     const ast::VariableDeclStatement* decl) {
     auto* sem_stmt = sem_expr->Stmt();
@@ -127,87 +116,6 @@ class PromoteSideEffectsToDecl::State {
     return false;
   }
 
-  // Hoists array and structure initializers to a constant variable, declared
-  // just before the statement of usage.
-  bool TypeConstructorToLet(const ast::CallExpression* expr) {
-    auto* ctor = ctx.src->Sem().Get(expr);
-    if (!ctor->Target()->Is<sem::TypeConstructor>()) {
-      return true;
-    }
-    auto* sem_stmt = ctor->Stmt();
-    if (!sem_stmt) {
-      // Expression is outside of a statement. This usually means the
-      // expression is part of a global (module-scope) constant declaration.
-      // These must be constexpr, and so cannot contain the type of
-      // expressions that must be sanitized.
-      return true;
-    }
-
-    auto* stmt = sem_stmt->Declaration();
-
-    if (auto* src_var_decl = stmt->As<ast::VariableDeclStatement>()) {
-      if (src_var_decl->variable->constructor == expr) {
-        // This statement is just a variable declaration with the
-        // initializer as the constructor value. This is what we're
-        // attempting to transform to, and so ignore.
-        return true;
-      }
-    }
-
-    auto* src_ty = ctor->Type();
-    if (!src_ty->IsAnyOf<sem::Array, sem::Struct>()) {
-      // We only care about array and struct initializers
-      return true;
-    }
-
-    // Construct the let that holds the hoisted initializer
-    auto name = b.Sym();
-    auto* let = b.Const(name, nullptr, ctx.Clone(expr));
-    auto* let_decl = b.Decl(let);
-
-    if (!InsertBefore(ctor, let_decl)) {
-      return false;
-    }
-
-    // Replace the initializer expression with a reference to the let
-    ctx.Replace(expr, b.Expr(name));
-    return true;
-  }
-
-  // Extracts array and matrix values that are dynamically indexed to a
-  // temporary `var` local that is then indexed.
-  bool DynamicIndexToVar(const ast::IndexAccessorExpression* access_expr) {
-    auto* index_expr = access_expr->index;
-    auto* object_expr = access_expr->object;
-    auto& sem = ctx.src->Sem();
-
-    if (sem.Get(index_expr)->ConstantValue()) {
-      // Index expression resolves to a compile time value.
-      // As this isn't a dynamic index, we can ignore this.
-      return true;
-    }
-
-    auto* indexed = sem.Get(object_expr);
-    if (!indexed->Type()->IsAnyOf<sem::Array, sem::Matrix>()) {
-      // We only care about array and matrices.
-      return true;
-    }
-
-    // Construct a `var` declaration to hold the value in memory.
-    // TODO(bclayton): group multiple accesses in the same object.
-    // e.g. arr[i] + arr[i+1] // Don't create two vars for this
-    auto var_name = b.Symbols().New("var_for_index");
-    auto* var_decl = b.Decl(b.Var(var_name, nullptr, ctx.Clone(object_expr)));
-
-    if (!InsertBefore(indexed, var_decl)) {
-      return false;
-    }
-
-    // Replace the original index expression with the new `var`.
-    ctx.Replace(object_expr, b.Expr(var_name));
-    return true;
-  }
-
   // Converts any for-loops marked for conversion to loops, inserting
   // registered declaration statements before the condition or continuing
   // statement.
@@ -315,9 +223,9 @@ class PromoteSideEffectsToDecl::State {
             auto& body_stmts = else_if_info.cond_decls;
 
             // Build nested if
-            body_stmts.emplace_back(b.If(ctx.Clone(else_stmt->condition),
+            auto* cond = ctx.Clone(else_stmt->condition);
-                                         ctx.Clone(else_stmt->body),
+            auto* body = ctx.Clone(else_stmt->body);
-                                         next_else_stmts));
+            body_stmts.emplace_back(b.If(cond, body, next_else_stmts));
 
             // Build else
             auto* else_with_nested_if = b.Else(b.Block(body_stmts));
@@ -334,87 +242,68 @@ class PromoteSideEffectsToDecl::State {
         TINT_ICE(Transform, b.Diagnostics())
             << "Expected else statements to insert into new if";
       }
-      auto* new_if = b.If(ctx.Clone(if_stmt->condition),
+      auto* cond = ctx.Clone(if_stmt->condition);
-                          ctx.Clone(if_stmt->body), next_else_stmts);
+      auto* body = ctx.Clone(if_stmt->body);
+      auto* new_if = b.If(cond, body, next_else_stmts);
       return new_if;
     });
   }
 
  public:
   /// Constructor
-  /// @param ctx_in the CloneContext primed with the input program and
+  /// @param ctx_in the clone context
-  /// @param cfg_in the transform config
+  explicit State(CloneContext& ctx_in) : ctx(ctx_in), b(*ctx_in.dst) {}
-  /// ProgramBuilder
-  explicit State(CloneContext& ctx_in, const Config& cfg_in)
-      : ctx(ctx_in), cfg(cfg_in), b(*ctx_in.dst) {}
 
-  /// Runs the transform
+  /// Hoists `expr` to a `let` or `var` with optional `decl_name`, inserting it
-  void Run() {
+  /// before `before_expr`.
-    // Scan the AST nodes for expressions that need to be promoted to their own
+  /// @param before_expr expression to insert `expr` before
-    // constant or variable declaration.
+  /// @param expr expression to hoist
+  /// @param as_const hoist to `let` if true, otherwise to `var`
+  /// @param decl_name optional name to use for the variable/constant name
+  /// @return true on success
+  bool HoistToDeclBefore(const sem::Expression* before_expr,
+                         const ast::Expression* expr,
+                         bool as_const,
+                         const char* decl_name = "") {
+    // Construct the let/var that holds the hoisted expr
+    auto name = b.Symbols().New(decl_name);
+    auto* v = as_const ? b.Const(name, nullptr, ctx.Clone(expr))
+                       : b.Var(name, nullptr, ctx.Clone(expr));
+    auto* decl = b.Decl(v);
 
-    // Note: Correct handling of nested expressions is guaranteed due to the
+    if (!InsertBefore(before_expr, decl)) {
-    // depth-first traversal of the ast::Node::Clone() methods:
+      return false;
-    //
-    // The inner-most expressions are traversed first, and they are hoisted
-    // to variables declared just above the statement of use. The outer
-    // expression will then be hoisted, inserting themselves between the
-    // inner declaration and the statement of use. This pattern applies
-    // correctly to any nested depth.
-    //
-    // Depth-first traversal of the AST is guaranteed because AST nodes are
-    // fully immutable and require their children to be constructed first so
-    // their pointer can be passed to the parent's constructor.
-
-    for (auto* node : ctx.src->ASTNodes().Objects()) {
-      if (cfg.type_ctor_to_let) {
-        if (auto* call_expr = node->As<ast::CallExpression>()) {
-          if (!TypeConstructorToLet(call_expr)) {
-            return;
-          }
-        }
-      }
-
-      if (cfg.dynamic_index_to_var) {
-        if (auto* access_expr = node->As<ast::IndexAccessorExpression>()) {
-          if (!DynamicIndexToVar(access_expr)) {
-            return;
-          }
-        }
-      }
     }
 
+    // Replace the initializer expression with a reference to the let
+    ctx.Replace(expr, b.Expr(name));
+    return true;
+  }
+
+  /// Applies any scheduled insertions from previous calls to Add() to
+  /// CloneContext. Call this once before ctx.Clone().
+  /// @return true on success
+  bool Apply() {
     ForLoopsToLoops();
     ElseIfsToElseWithNestedIfs();
-
+    return true;
-    ctx.Clone();
   }
 };
 
-PromoteSideEffectsToDecl::PromoteSideEffectsToDecl() = default;
+HoistToDeclBefore::HoistToDeclBefore(CloneContext& ctx)
-PromoteSideEffectsToDecl::~PromoteSideEffectsToDecl() = default;
+    : state_(std::make_unique<State>(ctx)) {}
 
-void PromoteSideEffectsToDecl::Run(CloneContext& ctx,
+HoistToDeclBefore::~HoistToDeclBefore() {}
-                                   const DataMap& inputs,
-                                   DataMap&) const {
-  auto* cfg = inputs.Get<Config>();
-  if (cfg == nullptr) {
-    ctx.dst->Diagnostics().add_error(
-        diag::System::Transform,
-        "missing transform data for " + std::string(TypeInfo().name));
-    return;
-  }
 
-  State state(ctx, *cfg);
+bool HoistToDeclBefore::Add(const sem::Expression* before_expr,
-  state.Run();
+                            const ast::Expression* expr,
+                            bool as_const,
+                            const char* decl_name) {
+  return state_->HoistToDeclBefore(before_expr, expr, as_const, decl_name);
 }
 
-PromoteSideEffectsToDecl::Config::Config(bool type_ctor_to_let_in,
+bool HoistToDeclBefore::Apply() {
-                                         bool dynamic_index_to_var_in)
+  return state_->Apply();
-    : type_ctor_to_let(type_ctor_to_let_in),
+}
-      dynamic_index_to_var(dynamic_index_to_var_in) {}
 
-PromoteSideEffectsToDecl::Config::~Config() = default;
+}  // namespace tint::transform
-
-}  // namespace transform
-}  // namespace tint

src/transform/utils/hoist_to_decl_before.h

@@ -0,0 +1,61 @@
+// 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_TRANSFORM_UTILS_HOIST_TO_DECL_BEFORE_H_
+#define SRC_TRANSFORM_UTILS_HOIST_TO_DECL_BEFORE_H_
+
+#include <memory>
+
+#include "src/sem/expression.h"
+#include "src/transform/transform.h"
+
+namespace tint::transform {
+
+/// Utility class that can be used to hoist expressions before other
+/// expressions, possibly converting 'for' loops to 'loop's and 'else if to
+// 'else if'.
+class HoistToDeclBefore {
+ public:
+  /// Constructor
+  /// @param ctx the clone context
+  explicit HoistToDeclBefore(CloneContext& ctx);
+
+  /// Destructor
+  ~HoistToDeclBefore();
+
+  /// Hoists `expr` to a `let` or `var` with optional `decl_name`, inserting it
+  /// before `before_expr`.
+  /// @param before_expr expression to insert `expr` before
+  /// @param expr expression to hoist
+  /// @param as_const hoist to `let` if true, otherwise to `var`
+  /// @param decl_name optional name to use for the variable/constant name
+  /// @return true on success
+  bool Add(const sem::Expression* before_expr,
+           const ast::Expression* expr,
+           bool as_const,
+           const char* decl_name = "");
+
+  /// Applies any scheduled insertions from previous calls to Add() to
+  /// CloneContext. Call this once before ctx.Clone().
+  /// @return true on success
+  bool Apply();
+
+ private:
+  class State;
+  std::unique_ptr<State> state_;
+};
+
+}  // namespace tint::transform
+
+#endif  // SRC_TRANSFORM_UTILS_HOIST_TO_DECL_BEFORE_H_

src/transform/utils/hoist_to_decl_before_test.cc

@@ -0,0 +1,221 @@
+// 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 <utility>
+
+#include "gtest/gtest-spi.h"
+#include "src/program_builder.h"
+#include "src/transform/test_helper.h"
+#include "src/transform/utils/hoist_to_decl_before.h"
+
+namespace tint::transform {
+namespace {
+
+using HoistToDeclBeforeTest = ::testing::Test;
+
+TEST_F(HoistToDeclBeforeTest, VarInit) {
+  // fn f() {
+  //     var a = 1;
+  // }
+  ProgramBuilder b;
+  auto* expr = b.Expr(1);
+  auto* var = b.Decl(b.Var("a", nullptr, expr));
+  b.Func("f", {}, b.ty.void_(), {var});
+
+  Program original(std::move(b));
+  ProgramBuilder cloned_b;
+  CloneContext ctx(&cloned_b, &original);
+
+  HoistToDeclBefore hoistToDeclBefore(ctx);
+  auto* sem_expr = ctx.src->Sem().Get(expr);
+  hoistToDeclBefore.Add(sem_expr, expr, true);
+  hoistToDeclBefore.Apply();
+
+  ctx.Clone();
+  Program cloned(std::move(cloned_b));
+
+  auto* expect = R"(
+fn f() {
+  let tint_symbol = 1;
+  var a = tint_symbol;
+}
+)";
+
+  EXPECT_EQ(expect, str(cloned));
+}
+
+TEST_F(HoistToDeclBeforeTest, ForLoopInit) {
+  // fn f() {
+  //     for(var a = 1; true; ) {
+  //     }
+  // }
+  ProgramBuilder b;
+  auto* expr = b.Expr(1);
+  auto* s =
+      b.For(b.Decl(b.Var("a", nullptr, expr)), b.Expr(true), {}, b.Block());
+  b.Func("f", {}, b.ty.void_(), {s});
+
+  Program original(std::move(b));
+  ProgramBuilder cloned_b;
+  CloneContext ctx(&cloned_b, &original);
+
+  HoistToDeclBefore hoistToDeclBefore(ctx);
+  auto* sem_expr = ctx.src->Sem().Get(expr);
+  hoistToDeclBefore.Add(sem_expr, expr, true);
+  hoistToDeclBefore.Apply();
+
+  ctx.Clone();
+  Program cloned(std::move(cloned_b));
+
+  auto* expect = R"(
+fn f() {
+  let tint_symbol = 1;
+  for(var a = tint_symbol; true; ) {
+  }
+}
+)";
+
+  EXPECT_EQ(expect, str(cloned));
+}
+
+TEST_F(HoistToDeclBeforeTest, ForLoopCond) {
+  // fn f() {
+  //     var a : bool;
+  //     for(; a; ) {
+  //     }
+  // }
+  ProgramBuilder b;
+  auto* var = b.Decl(b.Var("a", b.ty.bool_()));
+  auto* expr = b.Expr("a");
+  auto* s = b.For({}, expr, {}, b.Block());
+  b.Func("f", {}, b.ty.void_(), {var, s});
+
+  Program original(std::move(b));
+  ProgramBuilder cloned_b;
+  CloneContext ctx(&cloned_b, &original);
+
+  HoistToDeclBefore hoistToDeclBefore(ctx);
+  auto* sem_expr = ctx.src->Sem().Get(expr);
+  hoistToDeclBefore.Add(sem_expr, expr, true);
+  hoistToDeclBefore.Apply();
+
+  ctx.Clone();
+  Program cloned(std::move(cloned_b));
+
+  auto* expect = R"(
+fn f() {
+  var a : bool;
+  loop {
+    let tint_symbol = a;
+    if (!(tint_symbol)) {
+      break;
+    }
+    {
+    }
+  }
+}
+)";
+
+  EXPECT_EQ(expect, str(cloned));
+}
+
+TEST_F(HoistToDeclBeforeTest, ForLoopCont) {
+  // fn f() {
+  //     for(; true; var a = 1) {
+  //     }
+  // }
+  ProgramBuilder b;
+  auto* expr = b.Expr(1);
+  auto* s =
+      b.For({}, b.Expr(true), b.Decl(b.Var("a", nullptr, expr)), b.Block());
+  b.Func("f", {}, b.ty.void_(), {s});
+
+  Program original(std::move(b));
+  ProgramBuilder cloned_b;
+  CloneContext ctx(&cloned_b, &original);
+
+  HoistToDeclBefore hoistToDeclBefore(ctx);
+  auto* sem_expr = ctx.src->Sem().Get(expr);
+  hoistToDeclBefore.Add(sem_expr, expr, true);
+  hoistToDeclBefore.Apply();
+
+  ctx.Clone();
+  Program cloned(std::move(cloned_b));
+
+  auto* expect = R"(
+fn f() {
+  loop {
+    if (!(true)) {
+      break;
+    }
+    {
+    }
+
+    continuing {
+      let tint_symbol = 1;
+      var a = tint_symbol;
+    }
+  }
+}
+)";
+
+  EXPECT_EQ(expect, str(cloned));
+}
+
+TEST_F(HoistToDeclBeforeTest, ElseIf) {
+  // fn f() {
+  //     var a : bool;
+  //     if (true) {
+  //     } else if (a) {
+  //     } else {
+  //     }
+  // }
+  ProgramBuilder b;
+  auto* var = b.Decl(b.Var("a", b.ty.bool_()));
+  auto* expr = b.Expr("a");
+  auto* s = b.If(b.Expr(true), b.Block(),  //
+                 b.Else(expr, b.Block()),  //
+                 b.Else(b.Block()));
+  b.Func("f", {}, b.ty.void_(), {var, s});
+
+  Program original(std::move(b));
+  ProgramBuilder cloned_b;
+  CloneContext ctx(&cloned_b, &original);
+
+  HoistToDeclBefore hoistToDeclBefore(ctx);
+  auto* sem_expr = ctx.src->Sem().Get(expr);
+  hoistToDeclBefore.Add(sem_expr, expr, true);
+  hoistToDeclBefore.Apply();
+
+  ctx.Clone();
+  Program cloned(std::move(cloned_b));
+
+  auto* expect = R"(
+fn f() {
+  var a : bool;
+  if (true) {
+  } else {
+    let tint_symbol = a;
+    if (tint_symbol) {
+    } else {
+    }
+  }
+}
+)";
+
+  EXPECT_EQ(expect, str(cloned));
+}
+
+}  // namespace
+}  // namespace tint::transform

src/transform/var_for_dynamic_index.cc

@@ -0,0 +1,68 @@
+// 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/transform/var_for_dynamic_index.h"
+#include "src/program_builder.h"
+#include "src/transform/utils/hoist_to_decl_before.h"
+
+namespace tint::transform {
+
+VarForDynamicIndex::VarForDynamicIndex() = default;
+
+VarForDynamicIndex::~VarForDynamicIndex() = default;
+
+void VarForDynamicIndex::Run(CloneContext& ctx,
+                             const DataMap&,
+                             DataMap&) const {
+  HoistToDeclBefore hoist_to_decl_before(ctx);
+
+  // Extracts array and matrix values that are dynamically indexed to a
+  // temporary `var` local that is then indexed.
+  auto dynamic_index_to_var =
+      [&](const ast::IndexAccessorExpression* access_expr) {
+        auto* index_expr = access_expr->index;
+        auto* object_expr = access_expr->object;
+        auto& sem = ctx.src->Sem();
+
+        if (sem.Get(index_expr)->ConstantValue()) {
+          // Index expression resolves to a compile time value.
+          // As this isn't a dynamic index, we can ignore this.
+          return true;
+        }
+
+        auto* indexed = sem.Get(object_expr);
+        if (!indexed->Type()->IsAnyOf<sem::Array, sem::Matrix>()) {
+          // We only care about array and matrices.
+          return true;
+        }
+
+        // TODO(bclayton): group multiple accesses in the same object.
+        // e.g. arr[i] + arr[i+1] // Don't create two vars for this
+        return hoist_to_decl_before.Add(indexed, object_expr, false,
+                                        "var_for_index");
+      };
+
+  for (auto* node : ctx.src->ASTNodes().Objects()) {
+    if (auto* access_expr = node->As<ast::IndexAccessorExpression>()) {
+      if (!dynamic_index_to_var(access_expr)) {
+        return;
+      }
+    }
+  }
+
+  hoist_to_decl_before.Apply();
+  ctx.Clone();
+}
+
+}  // namespace tint::transform

src/transform/var_for_dynamic_index.h

@@ -0,0 +1,48 @@
+// 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_TRANSFORM_VAR_FOR_DYNAMIC_INDEX_H_
+#define SRC_TRANSFORM_VAR_FOR_DYNAMIC_INDEX_H_
+
+#include "src/transform/transform.h"
+
+namespace tint::transform {
+
+/// A transform that extracts array and matrix values that are dynamically
+/// indexed to a temporary `var` local before performing the index. This
+/// transform is used by the SPIR-V writer as there is no SPIR-V instruction
+/// that can dynamically index a non-pointer composite.
+class VarForDynamicIndex : public Transform {
+ public:
+  /// Constructor
+  VarForDynamicIndex();
+
+  /// Destructor
+  ~VarForDynamicIndex() override;
+
+ protected:
+  /// 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_TRANSFORM_VAR_FOR_DYNAMIC_INDEX_H_

src/transform/var_for_dynamic_index_test.cc

@@ -0,0 +1,553 @@
+// Copyright 2021 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/transform/var_for_dynamic_index.h"
+#include "src/transform/for_loop_to_loop.h"
+
+#include "src/transform/test_helper.h"
+
+namespace tint {
+namespace transform {
+namespace {
+
+using VarForDynamicIndexTest = TransformTest;
+
+TEST_F(VarForDynamicIndexTest, EmptyModule) {
+  auto* src = "";
+  auto* expect = "";
+
+  auto got = Run<ForLoopToLoop, VarForDynamicIndex>(src);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexDynamic) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 4>(1, 2, 3, 4);
+  let x = p[i];
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 4>(1, 2, 3, 4);
+  var var_for_index = p;
+  let x = var_for_index[i];
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexDynamic) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  let x = p[i];
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  var var_for_index = p;
+  let x = var_for_index[i];
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexDynamicChain) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  var j : i32;
+  let p = array<array<i32, 2>, 2>(array<i32, 2>(1, 2), array<i32, 2>(3, 4));
+  let x = p[i][j];
+}
+)";
+
+  // TODO(bclayton): Optimize this case:
+  // This output is not as efficient as it could be.
+  // We only actually need to hoist the inner-most array to a `var`
+  // (`var_for_index`), as later indexing operations will be working with
+  // references, not values.
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  var j : i32;
+  let p = array<array<i32, 2>, 2>(array<i32, 2>(1, 2), array<i32, 2>(3, 4));
+  var var_for_index = p;
+  var var_for_index_1 = var_for_index[i];
+  let x = var_for_index_1[j];
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexInForLoopInit) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = array<array<i32, 2>, 2>(array<i32, 2>(1, 2), array<i32, 2>(3, 4));
+  for(let x = p[i]; ; ) {
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = array<array<i32, 2>, 2>(array<i32, 2>(1, 2), array<i32, 2>(3, 4));
+  var var_for_index = p;
+  for(let x = var_for_index[i]; ; ) {
+    break;
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexInForLoopInit) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  for(let x = p[i]; ; ) {
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  var var_for_index = p;
+  for(let x = var_for_index[i]; ; ) {
+    break;
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexInForLoopCond) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 2>(1, 2);
+  for(; p[i] < 3; ) {
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 2>(1, 2);
+  loop {
+    var var_for_index = p;
+    if (!((var_for_index[i] < 3))) {
+      break;
+    }
+    {
+      break;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexInForLoopCond) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  for(; p[i].x < 3.0; ) {
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  loop {
+    var var_for_index = p;
+    if (!((var_for_index[i].x < 3.0))) {
+      break;
+    }
+    {
+      break;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexInForLoopCondWithNestedIndex) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  for(; p[i].x < 3.0; ) {
+    if (p[i].x < 1.0) {
+        var marker = 1;
+    }
+    break;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  loop {
+    var var_for_index = p;
+    if (!((var_for_index[i].x < 3.0))) {
+      break;
+    }
+    {
+      var var_for_index_1 = p;
+      if ((var_for_index_1[i].x < 1.0)) {
+        var marker = 1;
+      }
+      break;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexInElseIf) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 2>(1, 2);
+  if (false) {
+    var marker = 0;
+  } else if (p[i] < 3) {
+    var marker = 1;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 2>(1, 2);
+  if (false) {
+    var marker = 0;
+  } else {
+    var var_for_index = p;
+    if ((var_for_index[i] < 3)) {
+      var marker = 1;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexInElseIfChain) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 2>(1, 2);
+  if (true) {
+    var marker = 0;
+  } else if (true) {
+    var marker = 1;
+  } else if (p[i] < 3) {
+    var marker = 2;
+  } else if (p[i] < 4) {
+    var marker = 3;
+  } else if (true) {
+    var marker = 4;
+  } else {
+    var marker = 5;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = array<i32, 2>(1, 2);
+  if (true) {
+    var marker = 0;
+  } else if (true) {
+    var marker = 1;
+  } else {
+    var var_for_index = p;
+    if ((var_for_index[i] < 3)) {
+      var marker = 2;
+    } else {
+      var var_for_index_1 = p;
+      if ((var_for_index_1[i] < 4)) {
+        var marker = 3;
+      } else if (true) {
+        var marker = 4;
+      } else {
+        var marker = 5;
+      }
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexInElseIf) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  if (false) {
+    var marker_if = 1;
+  } else if (p[i].x < 3.0) {
+    var marker_else_if = 1;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  if (false) {
+    var marker_if = 1;
+  } else {
+    var var_for_index = p;
+    if ((var_for_index[i].x < 3.0)) {
+      var marker_else_if = 1;
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexInElseIfChain) {
+  auto* src = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  if (true) {
+    var marker = 0;
+  } else if (true) {
+    var marker = 1;
+  } else if (p[i].x < 3.0) {
+    var marker = 2;
+  } else if (p[i].y < 3.0) {
+    var marker = 3;
+  } else if (true) {
+    var marker = 4;
+  } else {
+    var marker = 5;
+  }
+}
+)";
+
+  auto* expect = R"(
+fn f() {
+  var i : i32;
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  if (true) {
+    var marker = 0;
+  } else if (true) {
+    var marker = 1;
+  } else {
+    var var_for_index = p;
+    if ((var_for_index[i].x < 3.0)) {
+      var marker = 2;
+    } else {
+      var var_for_index_1 = p;
+      if ((var_for_index_1[i].y < 3.0)) {
+        var marker = 3;
+      } else if (true) {
+        var marker = 4;
+      } else {
+        var marker = 5;
+      }
+    }
+  }
+}
+)";
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexLiteral) {
+  auto* src = R"(
+fn f() {
+  let p = array<i32, 4>(1, 2, 3, 4);
+  let x = p[1];
+}
+)";
+
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexLiteral) {
+  auto* src = R"(
+fn f() {
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  let x = p[1];
+}
+)";
+
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexConstantLet) {
+  auto* src = R"(
+fn f() {
+  let p = array<i32, 4>(1, 2, 3, 4);
+  let c = 1;
+  let x = p[c];
+}
+)";
+
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexConstantLet) {
+  auto* src = R"(
+fn f() {
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  let c = 1;
+  let x = p[c];
+}
+)";
+
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, ArrayIndexLiteralChain) {
+  auto* src = R"(
+fn f() {
+  let a = array<i32, 2>(1, 2);
+  let b = array<i32, 2>(3, 4);
+  let p = array<array<i32, 2>, 2>(a, b);
+  let x = p[0][1];
+}
+)";
+
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+TEST_F(VarForDynamicIndexTest, MatrixIndexLiteralChain) {
+  auto* src = R"(
+fn f() {
+  let p = mat2x2(1.0, 2.0, 3.0, 4.0);
+  let x = p[0][1];
+}
+)";
+
+  auto* expect = src;
+
+  DataMap data;
+  auto got = Run<VarForDynamicIndex>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
+}  // namespace
+}  // namespace transform
+}  // namespace tint

src/writer/hlsl/generator_impl.cc

@@ -58,7 +58,7 @@
 #include "src/transform/manager.h"
 #include "src/transform/num_workgroups_from_uniform.h"
 #include "src/transform/pad_array_elements.h"
-#include "src/transform/promote_side_effects_to_decl.h"
+#include "src/transform/promote_initializers_to_const_var.h"
 #include "src/transform/remove_phonies.h"
 #include "src/transform/simplify_pointers.h"
 #include "src/transform/unshadow.h"
@@ -192,10 +192,7 @@ SanitizedResult Sanitize(
   // will be transformed by CalculateArrayLength
   manager.Add<transform::CalculateArrayLength>();
   manager.Add<transform::ExternalTextureTransform>();
-
+  manager.Add<transform::PromoteInitializersToConstVar>();
-  data.Add<transform::PromoteSideEffectsToDecl::Config>(
-      /* type_ctor_to_let */ true, /* dynamic_index_to_var */ false);
-  manager.Add<transform::PromoteSideEffectsToDecl>();
 
   manager.Add<transform::PadArrayElements>();
   manager.Add<transform::AddEmptyEntryPoint>();

src/writer/msl/generator_impl.cc

@@ -64,7 +64,7 @@
 #include "src/transform/manager.h"
 #include "src/transform/module_scope_var_to_entry_point_param.h"
 #include "src/transform/pad_array_elements.h"
-#include "src/transform/promote_side_effects_to_decl.h"
+#include "src/transform/promote_initializers_to_const_var.h"
 #include "src/transform/remove_phonies.h"
 #include "src/transform/simplify_pointers.h"
 #include "src/transform/unshadow.h"
@@ -163,10 +163,7 @@ SanitizedResult Sanitize(
   }
   manager.Add<transform::CanonicalizeEntryPointIO>();
   manager.Add<transform::ExternalTextureTransform>();
-
+  manager.Add<transform::PromoteInitializersToConstVar>();
-  data.Add<transform::PromoteSideEffectsToDecl::Config>(
-      /* type_ctor_to_let */ true, /* dynamic_index_to_var */ false);
-  manager.Add<transform::PromoteSideEffectsToDecl>();
 
   manager.Add<transform::VectorizeScalarMatrixConstructors>();
   manager.Add<transform::WrapArraysInStructs>();

src/writer/spirv/builder.cc

@@ -48,10 +48,10 @@
 #include "src/transform/fold_constants.h"
 #include "src/transform/for_loop_to_loop.h"
 #include "src/transform/manager.h"
-#include "src/transform/promote_side_effects_to_decl.h"
 #include "src/transform/remove_unreachable_statements.h"
 #include "src/transform/simplify_pointers.h"
 #include "src/transform/unshadow.h"
+#include "src/transform/var_for_dynamic_index.h"
 #include "src/transform/vectorize_scalar_matrix_constructors.h"
 #include "src/transform/zero_init_workgroup_memory.h"
 #include "src/utils/defer.h"
@@ -272,10 +272,7 @@ SanitizedResult Sanitize(const Program* in,
   manager.Add<transform::CanonicalizeEntryPointIO>();
   manager.Add<transform::AddEmptyEntryPoint>();
   manager.Add<transform::AddSpirvBlockAttribute>();
-
+  manager.Add<transform::VarForDynamicIndex>();
-  data.Add<transform::PromoteSideEffectsToDecl::Config>(
-      /* type_ctor_to_let */ false, /* dynamic_index_to_var */ true);
-  manager.Add<transform::PromoteSideEffectsToDecl>();
 
   data.Add<transform::CanonicalizeEntryPointIO::Config>(
       transform::CanonicalizeEntryPointIO::Config(

test/BUILD.gn

@@ -324,7 +324,7 @@ tint_unittests_source_set("tint_unittests_transform_src") {
     "../src/transform/multiplanar_external_texture_test.cc",
     "../src/transform/num_workgroups_from_uniform_test.cc",
     "../src/transform/pad_array_elements_test.cc",
-    "../src/transform/promote_side_effects_to_decl_test.cc",
+    "../src/transform/promote_initializers_to_const_var_test.cc",
     "../src/transform/remove_phonies_test.cc",
     "../src/transform/remove_unreachable_statements_test.cc",
     "../src/transform/renamer_test.cc",
@@ -334,6 +334,8 @@ tint_unittests_source_set("tint_unittests_transform_src") {
     "../src/transform/test_helper.h",
     "../src/transform/transform_test.cc",
     "../src/transform/unshadow_test.cc",
+    "../src/transform/utils/hoist_to_decl_before_test.cc",
+    "../src/transform/var_for_dynamic_index_test.cc",
     "../src/transform/vectorize_scalar_matrix_constructors_test.cc",
     "../src/transform/vertex_pulling_test.cc",
     "../src/transform/wrap_arrays_in_structs_test.cc",