[Mlir-commits] [mlir] 253afd0 - [mlir][Interfaces] Symbols are not trivially dead

[Matthias Springer]

Author: Matthias Springer
Date: 2023-06-15T11:21:41+02:00
New Revision: 253afd03f1ffb9574109e36238b0854a214609c9

URL: https://github.com/llvm/llvm-project/commit/253afd03f1ffb9574109e36238b0854a214609c9
DIFF: https://github.com/llvm/llvm-project/commit/253afd03f1ffb9574109e36238b0854a214609c9.diff

LOG: [mlir][Interfaces] Symbols are not trivially dead

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.

Differential Revision: https://reviews.llvm.org/D152994

Added: 
    

Modified: 
    mlir/include/mlir/Interfaces/SideEffectInterfaces.h
    mlir/lib/Interfaces/SideEffectInterfaces.cpp
    mlir/test/IR/greedy-pattern-rewriter-driver.mlir
    mlir/test/Transforms/test-operation-folder-commutative.mlir
    mlir/test/lib/Dialect/Test/TestOps.td
    mlir/test/lib/Dialect/Test/TestPatterns.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/Interfaces/SideEffectInterfaces.h b/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
index ac42f38eb05e4..dfbb65ee6d488 100644
--- a/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
+++ b/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
@@ -316,6 +316,8 @@ 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.

diff  --git a/mlir/lib/Interfaces/SideEffectInterfaces.cpp b/mlir/lib/Interfaces/SideEffectInterfaces.cpp
index bff1517bf61cf..76ae27fab5a0c 100644
--- a/mlir/lib/Interfaces/SideEffectInterfaces.cpp
+++ b/mlir/lib/Interfaces/SideEffectInterfaces.cpp
@@ -8,6 +8,7 @@
 
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 
+#include "mlir/IR/SymbolTable.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
 using namespace mlir;
@@ -152,6 +153,8 @@ 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);
 }
 

diff  --git a/mlir/test/IR/greedy-pattern-rewriter-driver.mlir b/mlir/test/IR/greedy-pattern-rewriter-driver.mlir
index a4fb56d584a34..f3da9a147fcb9 100644
--- a/mlir/test/IR/greedy-pattern-rewriter-driver.mlir
+++ b/mlir/test/IR/greedy-pattern-rewriter-driver.mlir
@@ -24,3 +24,12 @@ func.func @add_ancestors_to_worklist() {
   }) {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"}> : () -> ()

diff  --git a/mlir/test/Transforms/test-operation-folder-commutative.mlir b/mlir/test/Transforms/test-operation-folder-commutative.mlir
index 89896e3bf99a8..8ffdeb54f399d 100644
--- a/mlir/test/Transforms/test-operation-folder-commutative.mlir
+++ b/mlir/test/Transforms/test-operation-folder-commutative.mlir
@@ -1,4 +1,4 @@
-// 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) {

diff  --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index cc53425df8339..8c8243af19ec0 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -106,7 +106,7 @@ def TEST_TestType : DialectType<Test_Dialect,
 // 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);

diff  --git a/mlir/test/lib/Dialect/Test/TestPatterns.cpp b/mlir/test/lib/Dialect/Test/TestPatterns.cpp
index 8a0ca056cd8b3..7a633415c83c8 100644
--- a/mlir/test/lib/Dialect/Test/TestPatterns.cpp
+++ b/mlir/test/lib/Dialect/Test/TestPatterns.cpp
@@ -200,7 +200,7 @@ struct HoistEligibleOps : public OpRewritePattern<test::OneRegionOp> {
 };
 
 struct TestPatternDriver
-    : public PassWrapper<TestPatternDriver, OperationPass<func::FuncOp>> {
+    : public PassWrapper<TestPatternDriver, OperationPass<ModuleOp>> {
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestPatternDriver)
 
   TestPatternDriver() = default;
@@ -1233,8 +1233,7 @@ struct RewriteDynamicOp : public RewritePattern {
 };
 
 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 {