[mlir][SideEffects] Replace the old SideEffects dialect interface with the newly added op interfaces/traits.

Summary:
The old interface was a temporary stopgap to allow for implementing simple LICM that took side effects of region operations into account. Now that MLIR has proper support for specifying memory effects, this interface can be deleted.

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

7 files changed, 29 insertions, 132 deletions

diff --git a/mlir/include/mlir/Dialect/AffineOps/AffineOps.td b/mlir/include/mlir/Dialect/AffineOps/AffineOps.td
index efbe02b..150d2a5 100644
--- a/mlir/include/mlir/Dialect/AffineOps/AffineOps.td
+++ b/mlir/include/mlir/Dialect/AffineOps/AffineOps.td
@@ -41,7 +41,7 @@ def ImplicitAffineTerminator
     : SingleBlockImplicitTerminator<"AffineTerminatorOp">;
 
 def AffineForOp : Affine_Op<"for",
-    [ImplicitAffineTerminator,
+    [ImplicitAffineTerminator, RecursiveSideEffects,
      DeclareOpInterfaceMethods<LoopLikeOpInterface>]> {
   let summary = "for operation";
   let description = [{
@@ -177,7 +177,8 @@ def AffineForOp : Affine_Op<"for",
   let hasFolder = 1;
 }
 
-def AffineIfOp : Affine_Op<"if", [ImplicitAffineTerminator]> {
+def AffineIfOp : Affine_Op<"if",
+                           [ImplicitAffineTerminator, RecursiveSideEffects]> {
   let summary = "if-then-else operation";
   let description = [{
     The "if" operation represents an if-then-else construct for conditionally
diff --git a/mlir/include/mlir/Dialect/LoopOps/LoopOps.td b/mlir/include/mlir/Dialect/LoopOps/LoopOps.td
index 28b2e8c..1ae328e 100644
--- a/mlir/include/mlir/Dialect/LoopOps/LoopOps.td
+++ b/mlir/include/mlir/Dialect/LoopOps/LoopOps.td
@@ -37,7 +37,8 @@ class Loop_Op<string mnemonic, list<OpTrait> traits = []> :
 
 def ForOp : Loop_Op<"for",
       [DeclareOpInterfaceMethods<LoopLikeOpInterface>,
-       SingleBlockImplicitTerminator<"YieldOp">]> {
+       SingleBlockImplicitTerminator<"YieldOp">,
+       RecursiveSideEffects]> {
   let summary = "for operation";
   let description = [{
     The "loop.for" operation represents a loop taking 3 SSA value as
@@ -170,7 +171,7 @@ def ForOp : Loop_Op<"for",
 }
 
 def IfOp : Loop_Op<"if",
-      [SingleBlockImplicitTerminator<"YieldOp">]> {
+      [SingleBlockImplicitTerminator<"YieldOp">, RecursiveSideEffects]> {
   let summary = "if-then-else operation";
   let description = [{
     The "loop.if" operation represents an if-then-else construct for
diff --git a/mlir/include/mlir/Transforms/SideEffectsInterface.h b/mlir/include/mlir/Transforms/SideEffectsInterface.h
deleted file mode 100644
index 517b7ae..0000000
--- a/mlir/include/mlir/Transforms/SideEffectsInterface.h
+++ /dev/null
@@ -1,64 +0,0 @@
-//===- SideEffectsInterface.h - dialect interface modeling side effects ---===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-//
-// This file specifies a dialect interface to model side-effects.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef MLIR_TRANSFORMS_SIDEEFFECTSINTERFACE_H_
-#define MLIR_TRANSFORMS_SIDEEFFECTSINTERFACE_H_
-
-#include "mlir/IR/DialectInterface.h"
-#include "mlir/IR/Operation.h"
-
-namespace mlir {
-
-/// Specifies an interface for basic side-effect modelling that is used by the
-/// loop-invariant code motion pass.
-///
-/// TODO: This interface should be replaced by a more general solution.
-class SideEffectsDialectInterface
-    : public DialectInterface::Base<SideEffectsDialectInterface> {
-public:
-  SideEffectsDialectInterface(Dialect *dialect) : Base(dialect) {}
-
-  enum SideEffecting {
-    Never,     /* the operation has no side-effects */
-    Recursive, /* the operation has side-effects if a contained operation has */
-    Always     /* the operation has side-effects */
-  };
-
-  /// Checks whether the given operation has side-effects.
-  virtual SideEffecting isSideEffecting(Operation *op) const {
-    if (op->hasNoSideEffect())
-      return Never;
-    return Always;
-  };
-};
-
-class SideEffectsInterface
-    : public DialectInterfaceCollection<SideEffectsDialectInterface> {
-public:
-  using SideEffecting = SideEffectsDialectInterface::SideEffecting;
-  explicit SideEffectsInterface(MLIRContext *ctx)
-      : DialectInterfaceCollection<SideEffectsDialectInterface>(ctx) {}
-
-  SideEffecting isSideEffecting(Operation *op) const {
-    // First check generic trait.
-    if (op->hasNoSideEffect())
-      return SideEffecting::Never;
-    if (auto handler = getInterfaceFor(op))
-      return handler->isSideEffecting(op);
-
-    return SideEffecting::Always;
-  }
-};
-
-} // namespace mlir
-
-#endif // MLIR_TRANSFORMS_SIDEEFFECTSINTERFACE_H_
diff --git a/mlir/lib/Dialect/AffineOps/AffineOps.cpp b/mlir/lib/Dialect/AffineOps/AffineOps.cpp
index 755a42b..7da6e25 100644
--- a/mlir/lib/Dialect/AffineOps/AffineOps.cpp
+++ b/mlir/lib/Dialect/AffineOps/AffineOps.cpp
@@ -15,7 +15,6 @@
 #include "mlir/IR/OpImplementation.h"
 #include "mlir/IR/PatternMatch.h"
 #include "mlir/Transforms/InliningUtils.h"
-#include "mlir/Transforms/SideEffectsInterface.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Debug.h"
@@ -62,19 +61,6 @@ struct AffineInlinerInterface : public DialectInlinerInterface {
   /// Affine regions should be analyzed recursively.
   bool shouldAnalyzeRecursively(Operation *op) const final { return true; }
 };
-
-// TODO(mlir): Extend for other ops in this dialect.
-struct AffineSideEffectsInterface : public SideEffectsDialectInterface {
-  using SideEffectsDialectInterface::SideEffectsDialectInterface;
-
-  SideEffecting isSideEffecting(Operation *op) const override {
-    if (isa<AffineIfOp>(op)) {
-      return Recursive;
-    }
-    return SideEffectsDialectInterface::isSideEffecting(op);
-  };
-};
-
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
@@ -88,7 +74,7 @@ AffineOpsDialect::AffineOpsDialect(MLIRContext *context)
 #define GET_OP_LIST
 #include "mlir/Dialect/AffineOps/AffineOps.cpp.inc"
                 >();
-  addInterfaces<AffineInlinerInterface, AffineSideEffectsInterface>();
+  addInterfaces<AffineInlinerInterface>();
 }
 
 /// Materialize a single constant operation from a given attribute value with
diff --git a/mlir/lib/Dialect/LoopOps/LoopOps.cpp b/mlir/lib/Dialect/LoopOps/LoopOps.cpp
index 9c28eec..4d1533a 100644
--- a/mlir/lib/Dialect/LoopOps/LoopOps.cpp
+++ b/mlir/lib/Dialect/LoopOps/LoopOps.cpp
@@ -20,30 +20,11 @@
 #include "mlir/IR/Value.h"
 #include "mlir/Support/MathExtras.h"
 #include "mlir/Support/STLExtras.h"
-#include "mlir/Transforms/SideEffectsInterface.h"
 
 using namespace mlir;
 using namespace mlir::loop;
 
 //===----------------------------------------------------------------------===//
-// LoopOpsDialect Interfaces
-//===----------------------------------------------------------------------===//
-namespace {
-
-struct LoopSideEffectsInterface : public SideEffectsDialectInterface {
-  using SideEffectsDialectInterface::SideEffectsDialectInterface;
-
-  SideEffecting isSideEffecting(Operation *op) const override {
-    if (isa<IfOp>(op) || isa<ForOp>(op)) {
-      return Recursive;
-    }
-    return SideEffectsDialectInterface::isSideEffecting(op);
-  };
-};
-
-} // namespace
-
-//===----------------------------------------------------------------------===//
 // LoopOpsDialect
 //===----------------------------------------------------------------------===//
 
@@ -53,7 +34,6 @@ LoopOpsDialect::LoopOpsDialect(MLIRContext *context)
 #define GET_OP_LIST
 #include "mlir/Dialect/LoopOps/LoopOps.cpp.inc"
       >();
-  addInterfaces<LoopSideEffectsInterface>();
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp b/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp
index 9ed95a9..ccac42f 100644
--- a/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp
+++ b/mlir/lib/Transforms/LoopInvariantCodeMotion.cpp
@@ -16,7 +16,6 @@
 #include "mlir/IR/Function.h"
 #include "mlir/Pass/Pass.h"
 #include "mlir/Transforms/LoopLikeInterface.h"
-#include "mlir/Transforms/SideEffectsInterface.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
@@ -27,8 +26,6 @@ using namespace mlir;
 
 namespace {
 
-using SideEffecting = SideEffectsInterface::SideEffecting;
-
 /// Loop invariant code motion (LICM) pass.
 struct LoopInvariantCodeMotion : public OperationPass<LoopInvariantCodeMotion> {
 public:
@@ -41,40 +38,39 @@ public:
 // - the op has no side-effects. If sideEffecting is Never, sideeffects of this
 //   op and its nested ops are ignored.
 static bool canBeHoisted(Operation *op,
-                         function_ref<bool(Value)> definedOutside,
-                         SideEffecting sideEffecting,
-                         SideEffectsInterface &interface) {
+                         function_ref<bool(Value)> definedOutside) {
   // Check that dependencies are defined outside of loop.
   if (!llvm::all_of(op->getOperands(), definedOutside))
     return false;
   // Check whether this op is side-effect free. If we already know that there
   // can be no side-effects because the surrounding op has claimed so, we can
   // (and have to) skip this step.
-  auto thisOpIsSideEffecting = sideEffecting;
-  if (thisOpIsSideEffecting != SideEffecting::Never) {
-    thisOpIsSideEffecting = interface.isSideEffecting(op);
-    // If the op always has sideeffects, we cannot hoist.
-    if (thisOpIsSideEffecting == SideEffecting::Always)
+  if (auto memInterface = dyn_cast<MemoryEffectOpInterface>(op)) {
+    if (!memInterface.hasNoEffect())
       return false;
+  } else if (!op->hasNoSideEffect() &&
+             !op->hasTrait<OpTrait::HasRecursiveSideEffects>()) {
+    return false;
   }
+
+  // If the operation doesn't have side effects and it doesn't recursively
+  // have side effects, it can always be hoisted.
+  if (!op->hasTrait<OpTrait::HasRecursiveSideEffects>())
+    return true;
+
   // Recurse into the regions for this op and check whether the contained ops
   // can be hoisted.
   for (auto &region : op->getRegions()) {
     for (auto &block : region.getBlocks()) {
-      for (auto &innerOp : block) {
-        if (innerOp.isKnownTerminator())
-          continue;
-        if (!canBeHoisted(&innerOp, definedOutside, thisOpIsSideEffecting,
-                          interface))
+      for (auto &innerOp : block.without_terminator())
+        if (!canBeHoisted(&innerOp, definedOutside))
           return false;
-      }
     }
   }
   return true;
 }
 
-static LogicalResult moveLoopInvariantCode(LoopLikeOpInterface looplike,
-                                           SideEffectsInterface &interface) {
+static LogicalResult moveLoopInvariantCode(LoopLikeOpInterface looplike) {
   auto &loopBody = looplike.getLoopBody();
 
   // We use two collections here as we need to preserve the order for insertion
@@ -94,9 +90,7 @@ static LogicalResult moveLoopInvariantCode(LoopLikeOpInterface looplike,
   // rewriting. If the nested regions are loops, they will have been processed.
   for (auto &block : loopBody) {
     for (auto &op : block.without_terminator()) {
-      if (canBeHoisted(&op, isDefinedOutsideOfBody,
-                       mlir::SideEffectsDialectInterface::Recursive,
-                       interface)) {
+      if (canBeHoisted(&op, isDefinedOutsideOfBody)) {
         opsToMove.push_back(&op);
         willBeMovedSet.insert(&op);
       }
@@ -113,16 +107,13 @@ static LogicalResult moveLoopInvariantCode(LoopLikeOpInterface looplike,
 } // end anonymous namespace
 
 void LoopInvariantCodeMotion::runOnOperation() {
-  SideEffectsInterface interface(&getContext());
   // Walk through all loops in a function in innermost-loop-first order. This
   // way, we first LICM from the inner loop, and place the ops in
   // the outer loop, which in turn can be further LICM'ed.
-  getOperation()->walk([&](Operation *op) {
-    if (auto looplike = dyn_cast<LoopLikeOpInterface>(op)) {
-      LLVM_DEBUG(op->print(llvm::dbgs() << "\nOriginal loop\n"));
-      if (failed(moveLoopInvariantCode(looplike, interface)))
-        signalPassFailure();
-    }
+  getOperation()->walk([&](LoopLikeOpInterface loopLike) {
+    LLVM_DEBUG(loopLike.print(llvm::dbgs() << "\nOriginal loop\n"));
+    if (failed(moveLoopInvariantCode(loopLike)))
+      signalPassFailure();
   });
 }
 
diff --git a/mlir/test/Transforms/loop-invariant-code-motion.mlir b/mlir/test/Transforms/loop-invariant-code-motion.mlir
index 1c39d56..5873532 100644
--- a/mlir/test/Transforms/loop-invariant-code-motion.mlir
+++ b/mlir/test/Transforms/loop-invariant-code-motion.mlir
@@ -105,6 +105,8 @@ func @invariant_affine_if() {
   // CHECK: %0 = alloc() : memref<10xf32>
   // CHECK-NEXT: %[[CST:.*]] = constant 8.000000e+00 : f32
   // CHECK-NEXT: affine.for %[[ARG:.*]] = 0 to 10 {
+  // CHECK-NEXT: }
+  // CHECK-NEXT: affine.for %[[ARG:.*]] = 0 to 10 {
   // CHECK-NEXT: affine.if #set0(%[[ARG]], %[[ARG]]) {
   // CHECK-NEXT: addf %[[CST]], %[[CST]] : f32
   // CHECK-NEXT: }