[Mlir-commits] [mlir] [mlir][bugfix] Fix erroneous condition in `getEffectsOnResource` (PR #133638)

[Andrzej Warzyński]

https://github.com/banach-space created https://github.com/llvm/llvm-project/pull/133638

This patch corrects an invalid condition in `getEffectsOnResource` used
to identify relevant "resources":

```cpp
return it.getResource() != resource;
```

The current implementation assumes that only one instance of each
resource will exist, so comparing raw pointers is both safe and
sufficient. This assumption stems from constructs like:

```cpp
static DerivedResource *get() {
  static DerivedResource instance;
  return &instance;
}
```
i.e., resource instances returned via static singleton methods.

However, as discussed in
 * https://github.com/llvm/llvm-project/issues/129216,
this assumption breaks in practice — notably on macOS (Apple Silicon)
when built with:

* `-DBUILD_SHARED_LIBS=On`.

In such cases, multiple instances of the same logical resource may exist
across shared library boundaries, leading to incorrect behavior and
causing failures in tests like:

* test/Dialect/Transform/check-use-after-free.mlir

This patch replaces the pointer comparison with a comparison based on
resource identity:

```cpp
return it.getResource()->getResourceID() != resource->getResourceID();
```
This approach aligns better with the intent of `getEffectsOnResource`,
which is to:

```cpp
/// Collect all of the effect instances that operate on the provided
/// resource (...)
```

Fixes #129216


>From 5bcd8f72c760140ed952634529ee8fc2353e863a Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Sun, 30 Mar 2025 12:29:10 +0100
Subject: [PATCH] [mlir][bugfix] Fix erroneous condition in
 `getEffectsOnResource`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This patch corrects an invalid condition in `getEffectsOnResource` used
to identify relevant "resources":

```cpp
return it.getResource() != resource;
```

The current implementation assumes that only one instance of each
resource will exist, so comparing raw pointers is both safe and
sufficient. This assumption stems from constructs like:

```cpp
static DerivedResource *get() {
  static DerivedResource instance;
  return &instance;
}
```
i.e., resource instances returned via static singleton methods.

However, as discussed in
 * https://github.com/llvm/llvm-project/issues/129216,
this assumption breaks in practice — notably on macOS (Apple Silicon)
when built with:

* `-DBUILD_SHARED_LIBS=On`.

In such cases, multiple instances of the same logical resource may exist
across shared library boundaries, leading to incorrect behavior and
causing failures in tests like:

* test/Dialect/Transform/check-use-after-free.mlir

This patch replaces the pointer comparison with a comparison based on
resource identity:

```cpp
return it.getResource()->getResourceID() != resource->getResourceID();
```
This approach aligns better with the intent of `getEffectsOnResource`,
which is to:

```cpp
/// Collect all of the effect instances that operate on the provided
/// resource (...)
```

Fixes #129216
---
 mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td b/mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td
index 45a9ffa94363e..043829c24fda8 100644
--- a/mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td
+++ b/mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td
@@ -140,7 +140,7 @@ class EffectOpInterfaceBase<string name, string baseEffect>
               }] # baseEffect # [{>> & effects) {
       getEffects(effects);
       ::llvm::erase_if(effects, [&](auto &it) {
-        return it.getResource() != resource;
+        return it.getResource()->getResourceID() != resource->getResourceID();
       });
     }
   }];