[Mlir-commits] [mlir] [mlir][Interfaces] Add `hasUnknownEffects` helper function (PR #154523)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-bufferization

Author: Matthias Springer (matthias-springer)

<details>
<summary>Changes</summary>

I have seen misuse of the `hasEffect` API in downstream projects: users something think that `hasEffect == false` indicates that the operation does not have a certain memory effect. That's not necessarily the case. When the op does not implement the `MemoryEffectsOpInterface`, it is unknown whether it has the specified effect. "false" can also mean "maybe".

This commit clarifies the semantics in the documentation. Also adds a `hasUnknownEffects` convenience function.


---
Full diff: https://github.com/llvm/llvm-project/pull/154523.diff


3 Files Affected:

- (modified) mlir/include/mlir/Interfaces/SideEffectInterfaces.h (+23-9) 
- (modified) mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp (+1-3) 
- (modified) mlir/lib/Interfaces/SideEffectInterfaces.cpp (+5) 


``````````diff
diff --git a/mlir/include/mlir/Interfaces/SideEffectInterfaces.h b/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
index aef7ec622fe4f..1e05001c2ed51 100644
--- a/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
+++ b/mlir/include/mlir/Interfaces/SideEffectInterfaces.h
@@ -383,33 +383,47 @@ struct Write : public Effect::Base<Write> {};
 // SideEffect Utilities
 //===----------------------------------------------------------------------===//
 
-/// Returns true if `op` has only an effect of type `EffectTy`.
+/// Returns "true" if `op` has only an effect of type `EffectTy`. Returns
+/// "false" if `op` has unknown effects or other/additional effects. Recursive
+/// effects are not taken into account.
 template <typename EffectTy>
 bool hasSingleEffect(Operation *op);
 
-/// Returns true if `op` has only an effect of type `EffectTy` (and of no other
-/// type) on `value`.
+/// Returns "true" if `op` has only an effect of type `EffectTy` on `value`.
+/// Returns "false" if `op` has unknown effects or other/additional effects.
+/// Recursive effects are not taken into account.
 template <typename EffectTy>
 bool hasSingleEffect(Operation *op, Value value);
 
-/// Returns true if `op` has only an effect of type `EffectTy` (and of no other
-/// type) on `value` of type `ValueTy`.
+/// Returns "true" if `op` has only an effect of type `EffectTy` on `value` of
+/// type `ValueTy`. Returns "false" if `op` has unknown effects or
+/// other/additional effects. Recursive effects are not taken into account.
 template <typename ValueTy, typename EffectTy>
 bool hasSingleEffect(Operation *op, ValueTy value);
 
-/// Returns true if `op` has an effect of type `EffectTy`.
+/// Returns "true" if `op` has an effect of type `EffectTy`. Returns "false" if
+/// `op` has unknown effects. Recursive effects are not taken into account.
 template <typename... EffectTys>
 bool hasEffect(Operation *op);
 
-/// Returns true if `op` has an effect of type `EffectTy` on `value`.
+/// Returns "true" if `op` has an effect of type `EffectTy` on `value`. Returns
+/// "false" if `op` has unknown effects. Recursive effects are not taken into
+/// account.
 template <typename... EffectTys>
 bool hasEffect(Operation *op, Value value);
 
-/// Returns true if `op` has an effect of type `EffectTy` on `value` of type
-/// `ValueTy`.
+/// Returns "true" if `op` has an effect of type `EffectTy` on `value` of type
+/// `ValueTy`. Returns "false" if `op` has unknown effects. Recursive effects
+/// are not taken into account.
 template <typename ValueTy, typename... EffectTys>
 bool hasEffect(Operation *op, ValueTy value);
 
+/// Return "true" if `op` has unknown effects. I.e., the effects of the
+/// operation itself are unknown and the operation does not derive its effects
+/// from its nested operations. (`HasRecursiveMemoryEffects` trait is not
+/// implemented or it is unknown whether it is implemented or not.)
+bool hasUnknownEffects(Operation *op);
+
 /// Return true if the given operation is unused, and has no side effects on
 /// memory that prevent erasing.
 bool isOpTriviallyDead(Operation *op);
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
index 725fa2429ce5b..6ffab9e39547f 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
@@ -517,9 +517,7 @@ LogicalResult BufferDeallocation::verifyOperationPreconditions(Operation *op) {
   //   MemoryEffectOpInterface. They usually do not have side effects apart
   //   from the callee, which will be analyzed separately. (This is similar to
   //   "recursive memory effects".)
-  if (!isa<MemoryEffectOpInterface>(op) &&
-      !op->hasTrait<OpTrait::HasRecursiveMemoryEffects>() &&
-      !isa<CallOpInterface>(op))
+  if (hasUnknownEffects(op) && !isa<CallOpInterface>(op))
     return op->emitError(
         "ops with unknown memory side effects are not supported");
 
diff --git a/mlir/lib/Interfaces/SideEffectInterfaces.cpp b/mlir/lib/Interfaces/SideEffectInterfaces.cpp
index 266f6dbacce89..b5a6888e5e1a4 100644
--- a/mlir/lib/Interfaces/SideEffectInterfaces.cpp
+++ b/mlir/lib/Interfaces/SideEffectInterfaces.cpp
@@ -304,6 +304,11 @@ template bool
 mlir::hasEffect<BlockArgument, MemoryEffects::Write, MemoryEffects::Free>(
     Operation *, BlockArgument);
 
+bool mlir::hasUnknownEffects(Operation *op) {
+  return !isa<MemoryEffectOpInterface>(op) &&
+         !op->hasTrait<OpTrait::HasRecursiveMemoryEffects>();
+}
+
 bool mlir::wouldOpBeTriviallyDead(Operation *op) {
   if (op->mightHaveTrait<OpTrait::IsTerminator>())
     return false;

``````````

</details>


https://github.com/llvm/llvm-project/pull/154523