Fixed a memory leak in the PDLToPDLInterp RootOrderingTest.

[sfuniak]

Summary

RootOrderingTest is a low-level unit test that creates values and uses them as vertices in a directed graph. These vertices were created using builder.create, but never freed, due to my insufficient understanding of the MLIR infrastructure.

[mehdi_amini]

Thanks for the fix!

It is a bit unusual to work with operations that aren't inserted in a block, I guess it works here...

mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp

27

51

Nit: in general I rather keep signed semantics as default.

[sfuniak]

It is a bit unusual to work with operations that aren't inserted in a block, I guess it works here...

Yes, @bondhugula raised the same point. The reason for creating the Values outside of the block is that they simply act as graph vertices / vertex IDs in RootOrdering. They are not being tested as a part of a block / IR. Maybe this warrants a comment on the unit test; will add it.

[sfuniak]

Review feedback.

[rriddle]

In D114745#3159842, @mehdi_amini wrote:

Thanks for the fix!

It is a bit unusual to work with operations that aren't inserted in a block, I guess it works here...

Would likely be better to have a Block next to the context field, and add the operations into that.

[sfuniak]

Add operations to a block to facilitate automatic deletion.

[mehdi_amini]

LG, thanks!

mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp

 //===- RootOrderingTest.cpp - unit tests for optimal branching ------------===//
 //
 // Part of the LLVM Project, under the Apache License v[1].0 with LLVM
 // Exceptions. See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../lib/Conversion/PDLToPDLInterp/RootOrdering.h"
-#include "mlir/Dialect/StandardOps/IR/Ops.h"
+#include "mlir/Dialect/Arithmetic/IR/Arithmetic.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/MLIRContext.h"
 #include "gtest/gtest.h"
 
 using namespace mlir;
+using namespace mlir::arith;
 using namespace mlir::pdl_to_pdl_interp;
 
 namespace {
 
 //===----------------------------------------------------------------------===//
 // Test Fixture
 //===----------------------------------------------------------------------===//
 
 /// The test fixture for constructing root ordering tests and verifying results.
 /// This fixture constructs the test values v. The test populates the graph
-/// with the desired costs and then calls check(), passing the expeted optimal
+/// with the desired costs and then calls check(), passing the expected optimal
 /// cost and the list of edges in the preorder traversal of the optimal
 /// branching.
 class RootOrderingTest : public ::testing::Test {
 protected:
   RootOrderingTest() {
-    context.loadDialect<StandardOpsDialect>();
+    context.loadDialect<ArithmeticDialect>();
     createValues();
   }
 
-  /// Creates the test values.
+  ~RootOrderingTest() {
+    for (int i = 0; i < 4; ++i) {
+      assert(v[i] && v[i].getDefiningOp() && "empty value / defining op");
+      v[i].getDefiningOp()->erase();
+    }
+  }
+
+  /// Creates the test values. These values simply act as vertices / vertex IDs
+  /// in the cost graph, rather than being a part of an IR. Therefore, we do not
+  /// insert them into a block. Consequently, they need to be manually erased in
+  /// the destructor above.
   void createValues() {
     OpBuilder builder(&context);
     for (int i = 0; i < 4; ++i)
-      v[i] = builder.create<ConstantOp>(builder.getUnknownLoc(),
-                                        builder.getI32IntegerAttr(i));
+      v[i] = builder.create<ConstantIntOp>(builder.getUnknownLoc(), i, 32);
   }
 
   /// Checks that optimal branching on graph has the given cost and
   /// its preorder traversal results in the specified edges.
   void check(unsigned cost, OptimalBranching::EdgeList edges) {
     OptimalBranching opt(graph, v[0]);
     EXPECT_EQ(opt.solve(), cost);
     EXPECT_EQ(opt.preOrderTraversal({v, v + edges.size()}), edges);