[transformer] Remove deprecated Manager constructor

Also moves the type determiner call out of the transformers into the
manager.
Cleans up the code to not have anything directly calling
Run() on the transformers other then the manager.

Bug: tint:308
Change-Id: I3343f2ba16dae6fb33f35e390ae4c797f2a05522
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/33262
Auto-Submit: Ryan Harrison <rharrison@chromium.org>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

samples/main.cc

@@ -507,7 +507,7 @@ int main(int argc, const char** argv) {
     return 1;
   }
 
-  tint::transform::Manager transform_manager;
+  tint::transform::Manager transform_manager(&ctx, &mod);
   for (const auto& name : options.transforms) {
     // TODO(dsinclair): The vertex pulling transform requires setup code to
     // be run that needs user input. Should we find a way to support that here

src/transform/bound_array_accessors_transform.h

@@ -40,6 +40,9 @@ class BoundArrayAccessorsTransform : public Transformer {
   explicit BoundArrayAccessorsTransform(Context* ctx, ast::Module* mod);
   ~BoundArrayAccessorsTransform() override;
 
+  /// Users of Tint should register the transform with transform manager and
+  /// invoke its Run(), instead of directly calling the transform's Run().
+  /// Calling Run() directly does not perform module state cleanup operations.
   /// @returns true if the transformation was successful
   bool Run() override;
 

src/transform/bound_array_accessors_transform_test.cc

@@ -41,6 +41,7 @@
 #include "src/ast/variable.h"
 #include "src/ast/variable_decl_statement.h"
 #include "src/context.h"
+#include "src/transform/manager.h"
 #include "src/type_determiner.h"
 
 namespace tint {
@@ -49,7 +50,13 @@ namespace {
 
 class BoundArrayAccessorsTest : public testing::Test {
  public:
-  BoundArrayAccessorsTest() : td_(&ctx_, &mod_), transform_(&ctx_, &mod_) {}
+  BoundArrayAccessorsTest() : td_(&ctx_, &mod_) {
+    auto transform =
+        std::make_unique<BoundArrayAccessorsTransform>(&ctx_, &mod_);
+    transform_ = transform.get();
+    manager_ = std::make_unique<Manager>(&ctx_, &mod_);
+    manager_->append(std::move(transform));
+  }
 
   ast::BlockStatement* SetupFunctionAndBody() {
     auto* block = create<ast::BlockStatement>();
@@ -67,7 +74,7 @@ class BoundArrayAccessorsTest : public testing::Test {
 
   TypeDeterminer* td() { return &td_; }
 
-  BoundArrayAccessorsTransform* transform() { return &transform_; }
+  Manager* manager() { return manager_.get(); }
 
   /// Creates a new `ast::Node` owned by the Context. When the Context is
   /// destructed, the `ast::Node` will also be destructed.
@@ -83,7 +90,8 @@ class BoundArrayAccessorsTest : public testing::Test {
   ast::Module mod_;
   TypeDeterminer td_;
   ast::type::VoidType void_type_;
-  BoundArrayAccessorsTransform transform_;
+  std::unique_ptr<Manager> manager_;
+  BoundArrayAccessorsTransform* transform_;
   ast::BlockStatement* body_ = nullptr;
 };
 
@@ -120,7 +128,7 @@ TEST_F(BoundArrayAccessorsTest, Ptrs_Clamp) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsCall());
 
@@ -183,7 +191,7 @@ TEST_F(BoundArrayAccessorsTest, Array_Idx_Nested_Scalar) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsCall());
 
@@ -256,7 +264,7 @@ TEST_F(BoundArrayAccessorsTest, Array_Idx_Scalar) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsConstructor());
   ASSERT_TRUE(ptr->idx_expr()->AsConstructor()->IsScalarConstructor());
@@ -304,7 +312,7 @@ TEST_F(BoundArrayAccessorsTest, Array_Idx_Expr) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsCall());
 
@@ -357,7 +365,7 @@ TEST_F(BoundArrayAccessorsTest, Array_Idx_Negative) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsConstructor());
   ASSERT_TRUE(ptr->idx_expr()->AsConstructor()->IsScalarConstructor());
@@ -396,7 +404,7 @@ TEST_F(BoundArrayAccessorsTest, Array_Idx_OutOfBounds) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsConstructor());
   ASSERT_TRUE(ptr->idx_expr()->AsConstructor()->IsScalarConstructor());
@@ -435,7 +443,7 @@ TEST_F(BoundArrayAccessorsTest, Vector_Idx_Scalar) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsConstructor());
   ASSERT_TRUE(ptr->idx_expr()->AsConstructor()->IsScalarConstructor());
@@ -483,7 +491,7 @@ TEST_F(BoundArrayAccessorsTest, Vector_Idx_Expr) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsCall());
 
@@ -535,7 +543,7 @@ TEST_F(BoundArrayAccessorsTest, Vector_Idx_Negative) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsConstructor());
   ASSERT_TRUE(ptr->idx_expr()->AsConstructor()->IsScalarConstructor());
@@ -574,7 +582,7 @@ TEST_F(BoundArrayAccessorsTest, Vector_Idx_OutOfBounds) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
   ASSERT_TRUE(ptr->idx_expr()->IsConstructor());
   ASSERT_TRUE(ptr->idx_expr()->AsConstructor()->IsScalarConstructor());
@@ -616,7 +624,7 @@ TEST_F(BoundArrayAccessorsTest, Matrix_Idx_Scalar) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
 
   ASSERT_TRUE(ptr->array()->IsArrayAccessor());
@@ -680,7 +688,7 @@ TEST_F(BoundArrayAccessorsTest, Matrix_Idx_Expr_Column) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
 
   ASSERT_TRUE(ptr->array()->IsArrayAccessor());
@@ -759,7 +767,7 @@ TEST_F(BoundArrayAccessorsTest, Matrix_Idx_Expr_Row) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
 
   ASSERT_TRUE(ptr->array()->IsArrayAccessor());
@@ -827,7 +835,7 @@ TEST_F(BoundArrayAccessorsTest, Matrix_Idx_Negative_Column) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
 
   ASSERT_TRUE(ptr->array()->IsArrayAccessor());
@@ -881,7 +889,7 @@ TEST_F(BoundArrayAccessorsTest, Matrix_Idx_Negative_Row) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
 
   ASSERT_TRUE(ptr->array()->IsArrayAccessor());
@@ -936,7 +944,7 @@ TEST_F(BoundArrayAccessorsTest, Matrix_Idx_OutOfBounds_Column) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
 
   ASSERT_TRUE(ptr->array()->IsArrayAccessor());
@@ -991,7 +999,7 @@ TEST_F(BoundArrayAccessorsTest, Matrix_Idx_OutOfBounds_Row) {
 
   ASSERT_TRUE(td()->Determine()) << td()->error();
 
-  ASSERT_TRUE(transform()->Run());
+  ASSERT_TRUE(manager()->Run());
   ASSERT_TRUE(ptr->IsArrayAccessor());
 
   ASSERT_TRUE(ptr->array()->IsArrayAccessor());

src/transform/manager.cc

@@ -19,8 +19,6 @@
 namespace tint {
 namespace transform {
 
-Manager::Manager() : context_(nullptr), module_(nullptr) {}
-
 Manager::Manager(Context* context, ast::Module* module)
     : context_(context), module_(module) {}
 

src/transform/manager.h

@@ -31,8 +31,6 @@ namespace transform {
 class Manager {
  public:
   /// Constructor
-  /// DEPRECATED
-  Manager();
   /// @param context the tint context
   /// @param module the module to transform
   Manager(Context* context, ast::Module* module);

src/transform/transformer.h

@@ -34,6 +34,9 @@ class Transformer {
   Transformer(Context* ctx, ast::Module* mod);
   virtual ~Transformer();
 
+  /// Users of Tint should register the transform with transform manager and
+  /// invoke its Run(), instead of directly calling the transform's Run().
+  /// Calling Run() directly does not perform module state cleanup operations.
   /// @returns true if the transformation was successful
   virtual bool Run() = 0;
 

src/transform/vertex_pulling_transform.cc

@@ -38,7 +38,6 @@
 #include "src/ast/type_constructor_expression.h"
 #include "src/ast/uint_literal.h"
 #include "src/ast/variable_decl_statement.h"
-#include "src/type_determiner.h"
 
 namespace tint {
 namespace transform {
@@ -101,11 +100,7 @@ bool VertexPullingTransform::Run() {
   AddVertexStorageBuffers();
   AddVertexPullingPreamble(vertex_func);
 
-  // We've potentially inserted nodes into the AST so we have to make sure to
+  return true;
-  // re-run type determination else those nodes will be missing their
-  // result_type
-  TypeDeterminer td(ctx_, mod_);
-  return td.Determine();
 }
 
 std::string VertexPullingTransform::GetVertexBufferName(uint32_t index) {

src/transform/vertex_pulling_transform.h

@@ -164,6 +164,9 @@ class VertexPullingTransform : public Transformer {
   /// @param number the set number we will use
   void SetPullingBufferBindingSet(uint32_t number);
 
+  /// Users of Tint should register the transform with transform manager and
+  /// invoke its Run(), instead of directly calling the transform's Run().
+  /// Calling Run() directly does not perform module state cleanup operations.
   /// @returns true if the transformation was successful
   bool Run() override;
 

src/transform/vertex_pulling_transform_test.cc

@@ -25,6 +25,7 @@
 #include "src/ast/type/f32_type.h"
 #include "src/ast/type/i32_type.h"
 #include "src/ast/type/void_type.h"
+#include "src/transform/manager.h"
 #include "src/type_determiner.h"
 #include "src/validator/validator.h"
 
@@ -36,7 +37,11 @@ class VertexPullingTransformHelper {
  public:
   VertexPullingTransformHelper() {
     mod_ = std::make_unique<ast::Module>();
-    transform_ = std::make_unique<VertexPullingTransform>(&ctx_, mod_.get());
+    manager_ = std::make_unique<Manager>(&ctx_, mod_.get());
+    auto transform =
+        std::make_unique<VertexPullingTransform>(&ctx_, mod_.get());
+    transform_ = transform.get();
+    manager_->append(std::move(transform));
   }
 
   // Create basic module with an entry point and vertex function
@@ -78,7 +83,8 @@ class VertexPullingTransformHelper {
 
   Context* ctx() { return &ctx_; }
   ast::Module* mod() { return mod_.get(); }
-  VertexPullingTransform* transform() { return transform_.get(); }
+  Manager* manager() { return manager_.get(); }
+  VertexPullingTransform* transform() { return transform_; }
 
   /// Creates a new `ast::Node` owned by the Context. When the Context is
   /// destructed, the `ast::Node` will also be destructed.
@@ -92,21 +98,22 @@ class VertexPullingTransformHelper {
  private:
   Context ctx_;
   std::unique_ptr<ast::Module> mod_;
-  std::unique_ptr<VertexPullingTransform> transform_;
+  std::unique_ptr<Manager> manager_;
+  VertexPullingTransform* transform_;
 };
 
 class VertexPullingTransformTest : public VertexPullingTransformHelper,
                                    public testing::Test {};
 
 TEST_F(VertexPullingTransformTest, Error_NoVertexState) {
-  EXPECT_FALSE(transform()->Run());
+  EXPECT_FALSE(manager()->Run());
-  EXPECT_EQ(transform()->error(), "SetVertexState not called");
+  EXPECT_EQ(manager()->error(), "SetVertexState not called");
 }
 
 TEST_F(VertexPullingTransformTest, Error_NoEntryPoint) {
   transform()->SetVertexState(std::make_unique<VertexStateDescriptor>());
-  EXPECT_FALSE(transform()->Run());
+  EXPECT_FALSE(manager()->Run());
-  EXPECT_EQ(transform()->error(), "Vertex stage entry point not found");
+  EXPECT_EQ(manager()->error(), "Vertex stage entry point not found");
 }
 
 TEST_F(VertexPullingTransformTest, Error_InvalidEntryPoint) {
@@ -114,8 +121,8 @@ TEST_F(VertexPullingTransformTest, Error_InvalidEntryPoint) {
   InitTransform({});
   transform()->SetEntryPoint("_");
 
-  EXPECT_FALSE(transform()->Run());
+  EXPECT_FALSE(manager()->Run());
-  EXPECT_EQ(transform()->error(), "Vertex stage entry point not found");
+  EXPECT_EQ(manager()->error(), "Vertex stage entry point not found");
 }
 
 TEST_F(VertexPullingTransformTest, Error_EntryPointWrongStage) {
@@ -128,14 +135,14 @@ TEST_F(VertexPullingTransformTest, Error_EntryPointWrongStage) {
   mod()->AddFunction(func);
 
   InitTransform({});
-  EXPECT_FALSE(transform()->Run());
+  EXPECT_FALSE(manager()->Run());
-  EXPECT_EQ(transform()->error(), "Vertex stage entry point not found");
+  EXPECT_EQ(manager()->error(), "Vertex stage entry point not found");
 }
 
 TEST_F(VertexPullingTransformTest, BasicModule) {
   InitBasicModule();
   InitTransform({});
-  EXPECT_TRUE(transform()->Run());
+  EXPECT_TRUE(manager()->Run());
 }
 
 TEST_F(VertexPullingTransformTest, OneAttribute) {
@@ -146,7 +153,7 @@ TEST_F(VertexPullingTransformTest, OneAttribute) {
 
   InitTransform({{{4, InputStepMode::kVertex, {{VertexFormat::kF32, 0, 0}}}}});
 
-  EXPECT_TRUE(transform()->Run());
+  EXPECT_TRUE(manager()->Run());
 
   EXPECT_EQ(R"(Module{
   TintVertexData Struct{
@@ -231,7 +238,7 @@ TEST_F(VertexPullingTransformTest, OneInstancedAttribute) {
   InitTransform(
       {{{4, InputStepMode::kInstance, {{VertexFormat::kF32, 0, 0}}}}});
 
-  EXPECT_TRUE(transform()->Run());
+  EXPECT_TRUE(manager()->Run());
 
   EXPECT_EQ(R"(Module{
   TintVertexData Struct{
@@ -316,7 +323,7 @@ TEST_F(VertexPullingTransformTest, OneAttributeDifferentOutputSet) {
   InitTransform({{{4, InputStepMode::kVertex, {{VertexFormat::kF32, 0, 0}}}}});
   transform()->SetPullingBufferBindingSet(5);
 
-  EXPECT_TRUE(transform()->Run());
+  EXPECT_TRUE(manager()->Run());
 
   EXPECT_EQ(R"(Module{
   TintVertexData Struct{
@@ -431,7 +438,7 @@ TEST_F(VertexPullingTransformTest, ExistingVertexIndexAndInstanceIndex) {
       {{{4, InputStepMode::kVertex, {{VertexFormat::kF32, 0, 0}}},
         {4, InputStepMode::kInstance, {{VertexFormat::kF32, 0, 1}}}}});
 
-  EXPECT_TRUE(transform()->Run());
+  EXPECT_TRUE(manager()->Run());
 
   EXPECT_EQ(R"(Module{
   TintVertexData Struct{
@@ -571,7 +578,7 @@ TEST_F(VertexPullingTransformTest, TwoAttributesSameBuffer) {
          InputStepMode::kVertex,
          {{VertexFormat::kF32, 0, 0}, {VertexFormat::kVec4F32, 0, 1}}}}});
 
-  EXPECT_TRUE(transform()->Run());
+  EXPECT_TRUE(manager()->Run());
 
   EXPECT_EQ(R"(Module{
   TintVertexData Struct{
@@ -756,7 +763,7 @@ TEST_F(VertexPullingTransformTest, FloatVectorAttributes) {
         {12, InputStepMode::kVertex, {{VertexFormat::kVec3F32, 0, 1}}},
         {16, InputStepMode::kVertex, {{VertexFormat::kVec4F32, 0, 2}}}}});
 
-  EXPECT_TRUE(transform()->Run());
+  EXPECT_TRUE(manager()->Run());
 
   EXPECT_EQ(R"(Module{
   TintVertexData Struct{