[mlir][Interfaces] Symbols are not trivially dead

[springerm]

Summary

The greedy pattern rewrite driver removes ops that are "trivially dead". This could include symbols that are still referenced by other ops. Dead symbols should be removed with the -symbol-dce pass instead.

This bug was not triggered for func::FuncOp, because ops are not considered "trivally dead" if they do not implement the MemoryEffectOpInterface, indicating that the op may or may not have side effects. It is, however, triggered for transform::NamedSequenceOp, which implements that interface because it is required for all transform dialect ops.

[mehdi_amini]

mlir/test/Dialect/Transform/test-interpreter.mlir

5 ↗

(On Diff #531626)

This isn't the kind of test that is high value, can you add a targeted test instead please?

[springerm]

address comments

[springerm]

fix test

[mehdi_amini]

mlir/test/lib/Dialect/Test/TestPatterns.cpp

203

Could have made this just and OperationPass without restricting to a specific op?

[springerm]

mlir/test/lib/Dialect/Test/TestPatterns.cpp

203

I removed it from the wrong pass, here's a revision that removes it from all passes in this file when possible: D153005

mlir/include/mlir/Interfaces/SideEffectInterfaces.h

 
 /// Return true if the given operation is unused, and has no side effects on
 /// memory that prevent erasing.
 bool isOpTriviallyDead(Operation *op);
 
 /// Return true if the given operation would be dead if unused, and has no side
 /// effects on memory that would prevent erasing. This is equivalent to checking
 /// `isOpTriviallyDead` if `op` was unused.
+///
+/// Note: Terminators and symbols are never considered to be trivially dead.
 bool wouldOpBeTriviallyDead(Operation *op);
 
 /// Returns true if the given operation is free of memory effects.
 ///
 /// An operation is free of memory effects if its implementation of
 /// `MemoryEffectOpInterface` indicates that it has no memory effects. For
 /// example, it may implement `NoMemoryEffect` in ODS. Alternatively, if the
 /// operation has the `HasRecursiveMemoryEffects` trait, then it is free of

mlir/lib/Interfaces/SideEffectInterfaces.cpp

 //===- SideEffectInterfaces.cpp - SideEffects in MLIR ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 
+#include "mlir/IR/SymbolTable.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
 using namespace mlir;
 
 //===----------------------------------------------------------------------===//
 // SideEffect Interfaces
 //===----------------------------------------------------------------------===//
 

 template bool mlir::hasEffect<MemoryEffects::Read>(Operation *, Value);
 template bool mlir::hasEffect<MemoryEffects::Write>(Operation *, Value);
 template bool
 mlir::hasEffect<MemoryEffects::Write, MemoryEffects::Free>(Operation *, Value);
 
 bool mlir::wouldOpBeTriviallyDead(Operation *op) {
   if (op->mightHaveTrait<OpTrait::IsTerminator>())
     return false;
+  if (isa<SymbolOpInterface>(op))
+    return false;
   return wouldOpBeTriviallyDeadImpl(op);
 }
 
 bool mlir::isMemoryEffectFree(Operation *op) {
   if (auto memInterface = dyn_cast<MemoryEffectOpInterface>(op)) {
     if (!memInterface.hasNoEffect())
       return false;
     // If the op does not have recursive side effects, then it is memory effect

mlir/test/IR/greedy-pattern-rewriter-driver.mlir

func.func @add_ancestors_to_worklist() {
        // CHECK: "foo.maybe_eligible_op"() {eligible} : () -> index
   // CHECK-NEXT: "test.one_region_op"()
   "test.one_region_op"() ({
     %0 = "foo.maybe_eligible_op" () : () -> (index)
     "foo.yield"(%0) : (index) -> ()
   }) {hoist_eligible_ops}: () -> ()
   return
 }
+
+// -----
+
+// There are no patterns in this test that apply to "test.symbol". This test is
+// to ensure that symbols are not getting removed due to being "trivially dead"
+// as part of a greedy rewrite. Symbols are never trivially dead.
+
+// CHECK: "test.symbol"() <{sym_name = "foo"}>
+"test.symbol"() <{sym_name = "foo"}> : () -> ()

mlir/test/Transforms/test-operation-folder-commutative.mlir

-// RUN: mlir-opt --pass-pipeline="builtin.module(func.func(test-patterns))" %s | FileCheck %s
+// RUN: mlir-opt --pass-pipeline="builtin.module(test-patterns)" %s | FileCheck %s
 
 // CHECK-LABEL: func @test_reorder_constants_and_match
 func.func @test_reorder_constants_and_match(%arg0 : i32) -> (i32) {
   // CHECK: %[[CST:.+]] = arith.constant 43
   %cst = arith.constant 43 : i32
   // CHECK: return %[[CST]]
   %y = "test.op_commutative2"(%cst, %arg0) : (i32, i32) -> i32
   %x = "test.op_commutative2"(%y, %arg0) : (i32, i32) -> i32
   return %x : i32
 }

mlir/test/lib/Dialect/Test/TestOps.td

 def TEST_TestType : DialectType<Test_Dialect,
     CPred<"::llvm::isa<::test::TestType>($_self)">, "test">,
     BuildableType<"$_builder.getType<::test::TestType>()">;
 
 //===----------------------------------------------------------------------===//
 // Test Symbols
 //===----------------------------------------------------------------------===//
 
-def SymbolOp : TEST_Op<"symbol", [Symbol]> {
+def SymbolOp : TEST_Op<"symbol", [NoMemoryEffect, Symbol]> {
   let summary =  "operation which defines a new symbol";
   let arguments = (ins StrAttr:$sym_name,
                        OptionalAttr<StrAttr>:$sym_visibility);
 }
 
 def SymbolScopeOp : TEST_Op<"symbol_scope",
     [SymbolTable, SingleBlockImplicitTerminator<"TerminatorOp">]> {
   let summary =  "operation which defines a new symbol table";

mlir/test/lib/Dialect/Test/TestPatterns.cpp

LogicalResult matchAndRewrite(test::OneRegionOp op,
     // Hoisting means removing an op from the enclosing op. I.e., the enclosing
     // op is modified.
     rewriter.updateRootInPlace(op, [&]() { toBeHoisted->moveBefore(op); });
     return success();
   }
 };
 
 struct TestPatternDriver
-    : public PassWrapper<TestPatternDriver, OperationPass<func::FuncOp>> {
+    : public PassWrapper<TestPatternDriver, OperationPass<ModuleOp>> {
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestPatternDriver)
 
   TestPatternDriver() = default;
   TestPatternDriver(const TestPatternDriver &other) : PassWrapper(other) {}
 
   StringRef getArgument() const final { return "test-patterns"; }
   StringRef getDescription() const final { return "Run test dialect patterns"; }
   void runOnOperation() override {
OperationState state(op->getLoc(), "test.dynamic_generic",
                          op->getAttrs());
     auto *newOp = rewriter.create(state);
     rewriter.replaceOp(op, newOp->getResults());
     return success();
   }
 };
 
 struct TestRewriteDynamicOpDriver
-    : public PassWrapper<TestRewriteDynamicOpDriver,
-                         OperationPass<func::FuncOp>> {
+    : public PassWrapper<TestRewriteDynamicOpDriver, OperationPass<>> {
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestRewriteDynamicOpDriver)
 
   void getDependentDialects(DialectRegistry &registry) const override {
     registry.insert<TestDialect>();
   }
   StringRef getArgument() const final { return "test-rewrite-dynamic-op"; }
   StringRef getDescription() const final {
     return "Test rewritting on dynamic operations";