[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

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 ↗

(On Diff #531635)

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

[springerm]

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

203 ↗

(On Diff #531635)

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/Dialect/Transform/test-interpreter.mlir

 // RUN: mlir-opt %s --test-transform-dialect-interpreter -allow-unregistered-dialect --split-input-file --verify-diagnostics | FileCheck %s
 
+// Make sure that canonicalizing the IR (including transform IR) does not result
+// in invalid IR.
+// RUN: mlir-opt %s -canonicalize -allow-unregistered-dialect --split-input-file
+
 transform.sequence failures(propagate) {
 ^bb0(%arg0: !transform.any_op):
   // expected-remark @below {{applying transformation}}
   transform.test_transform_op
 }
 
 // -----