[mlir][bugfix] Fix erroneous condition in getEffectsOnResource
#133638
Conversation
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
* llvm#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 llvm#129216
|
@llvm/pr-subscribers-mlir Author: Andrzej Warzyński (banach-space) ChangesThis patch corrects an invalid condition in return it.getResource() != resource;The current implementation assumes that only one instance of each static DerivedResource *get() {
static DerivedResource instance;
return &instance;
}i.e., resource instances returned via static singleton methods. However, as discussed in
In such cases, multiple instances of the same logical resource may exist
This patch replaces the pointer comparison with a comparison based on return it.getResource()->getResourceID() != resource->getResourceID();This approach aligns better with the intent of /// Collect all of the effect instances that operate on the provided
/// resource (...)Fixes #129216 Full diff: https://github.com/llvm/llvm-project/pull/133638.diff 1 Files Affected:
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();
});
}
}];
|
|
Thanks for taking a look, Alex! 🙏
Sure! If I understand correctly, this comes down to where the ID is "registered". That happens in ImplicitTypeIDRegistry, which is decorated with To add more context, let me share what makes
I hope that clarifies things and that I didn't miss anything here. |
|
Since there are no new comments, I assume this is good to land. |
This patch corrects an invalid condition in
getEffectsOnResourceusedto identify relevant "resources":
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:
i.e., resource instances returned via static singleton methods.
However, as discussed in
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:
This patch replaces the pointer comparison with a comparison based on
resource identity:
This approach aligns better with the intent of
getEffectsOnResource,which is to:
Fixes #129216