llvm / llvm/llvm-project

[mlir] Side-effect Resource disjointness relies on cross-image singleton identity — silent miscompile in multi-DSO builds

Open
#219,579 1 comment 0 reactions 1 assignee Claimed by @vzakhari View on GitHub
mlir
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

(This issue was created with the help of AI. I checked it to the best of my knowledge.)

## Summary

Since #181229 ("Resource hierarchy for MLIR Side Effects"), CSE refines its
intervening-write check by *resource disjointness*: a write whose resource is
`isDisjointFrom` the read's resource no longer blocks CSE of two identical
read-only ops (`mlir/lib/Transforms/Utils/CSE.cpp`,
`hasOtherSideEffectingOpInBetween`).

Disjointness bottoms out in **instance pointer identity** over resource
singletons:

```cpp
// mlir/include/mlir/Interfaces/SideEffectInterfaces.h
bool isSubresourceOf(const Resource *other) const {
for (const Resource *r = this; r != nullptr; r = r->getParent())
if (r == other) // <-- pointer identity
return true;
return false;
}
bool isDisjointFrom(const Resource *other) const {
return !isSubresourceOf(other) && !other->isSubresourceOf(this);
}
```

and the singleton comes from an **inline template member with a function-local
static**, which has no anchoring/export machinery (unlike op/dialect TypeIDs,
which have `MLIR_DECLARE_EXPLICIT_TYPE_ID`):

```cpp
// Resource::Base
static DerivedResource *get() {
static DerivedResource instance; // one copy PER IMAGE in a multi-DSO process
return &instance;
}
```

In a process where MLIR code is duplicated across several shared libraries,
each image can end up with its own `DefaultResource` instance.
`DefaultResource::getParent()` is `nullptr`, so `isDisjointFrom` between two
copies returns **true**, and CSE concludes an intervening write cannot
conflict with the reads — **silently producing wrong code**. Before #181229
the check was conservative (any intervening write blocked CSE), so duplicated
singletons were harmless; the refinement converted a long-standing
loud-or-benign build hazard into a silent miscompile with no diagnostic.

## Field case

#219450: Homebrew's flang 23.1.0 bottle for macOS/arm64 (a *standalone* flang
build with `BUILD_SHARED_LIBS=ON` and LTO, linking the llvm package's
`libMLIR.dylib`/`libLLVM.dylib`) miscompiles

```fortran
f = 72.d0
f = sqrt(f) ! dropped: the function returns 72.0 at -O1+
```

`fir-opt --cse` on that build merges two `fir.load`s across an intervening
`hlfir.assign`, because:

- `hlfir::AssignOp::getEffects` (custom C++, in flang's HLFIR library) reports
its Write with `SideEffects::DefaultResource::get()` evaluated in the
**HLFIR image**;
- `fir.load`'s declarative `MemRead` materializes `DefaultResource` in the
**FIR image**;
- on Mach-O (two-level namespace, plus LTO internalization) those are
different instances → "disjoint" → the loads merge.

The same build handles `fir.store` correctly — `fir.load`/`fir.store` effects
materialize in the *same* image, so the singleton matches and the conflict is
detected. That asymmetry (assign broken, store fine) is reproducible on the
affected build with a 20-line `.mlir` file (see #219450).

The same sources built on Linux are correct in every configuration I could
construct (monorepo static; standalone `BUILD_SHARED_LIBS=ON` against an
`LLVM_LINK_LLVM_DYLIB=ON` install): ELF coalesces the weak static process-wide.
A standalone shared build against a fully-static LLVM instead **aborts at
startup** with `Option '...' registered more than once! LLVM ERROR:
inconsistency in registered CommandLine options` — the *loud* sibling of the
same duplication family, long known from e.g. #60601, #58466, #62125, #34508,
#60888, #43444, and on the MLIR side #162426 (ASAN ODR violations under
`LLVM_LINK_LLVM_DYLIB=ON`). The resource-singleton variant is the first member
of the family that silently miscompiles instead of failing loudly.

## Suggested fixes

Either of these would make duplication degrade back to conservatism instead of
wrong code:

1. **Anchor the resource singletons**: move `Resource::Base::get()`'s
definition out-of-line (exported from the MLIR library) at least for the
built-in resources (`DefaultResource`,
`AutomaticAllocationScopeResource`, ...), and document the anchoring
requirement for downstream-defined resources — mirroring what
`MLIR_DECLARE_EXPLICIT_TYPE_ID` does for TypeIDs.
2. **Compare resources by `getResourceID()`** (TypeID, which has the fallback
resolver path designed for shared-library use) rather than instance
pointers in `isSubresourceOf`.

Verified at `llvmorg-23.1.0` and current main (`ad3c19a89842`); the
pointer-identity comparison and the unanchored `get()` are present in both.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.