[Mlir-commits] [mlir] [mlir][CSE] Eliminate redundant reads across dominating blocks (PR #218146)

[llvmlistbot at llvm.org]

prometheusfma-llvm wrote:

> * As I understand it, this is fundamentally a read/write constraint issue. Here, we have two cases: Read-After-Read (RAR) with no intervening writes, and Write-After-Write (WAW) with no intervening reads. I think both cases can be handled together since their underlying logic is essentially the same. Therefore, instead of shoehorning this into CSE, it might make more sense to implement a standalone memory optimization pass dedicated to these eliminations.
> * The implementation in this PR doesn't feel particularly clean to me (though to be fair, I've only skimmed through it). My understanding is that you simply need to update the MemEffectsCache continuously as you traverse from fromOp to nextOp—the key lies in how the cache is updated. When moving from fromOp to the next op, if that op contains regions, you should step into the region and check for writes. Once you hit a terminator op, you exit the region and resume traversal until you reach toOp.



Thanks for taking a look, and for the RAR/WAW framing — that's a nice way to think about it.

**On scope (standalone pass vs CSE).** I agree RAR and WAW are duals at the level of "is there an intervening conflicting effect." Worth noting they're still different rewrites: the RAR case here keeps the first read and drops the second (exactly what CSE already does for MemRead within a block), whereas WAW/dead-store elimination drops the *first* write and additionally needs "no read between" plus the result being unused. So they'd share the intervening-effect scan but not the rewrite itself. This PR is deliberately scoped as an incremental extension of the MemRead handling CSE already has (introduced in 02da964, extended for recursive effects in dea33c8 and for the resource hierarchy in 48e6adc), rather than a new pass. I'm happy to go the standalone-pass route if that's the consensus, but that feels RFC-scale (there's no generic memory-SSA-based pass in core today) and orthogonal to fixing this specific missed-opt. Do you feel strongly it must be a separate pass to land?

**On region stepping.** Good news — this is already handled. The intervening-write scan goes through `getEffectsRecursively`, which recurses into the regions of any op on the path (via `HasRecursiveMemoryEffects`), so an `scf.if`/`scf.for` between the two reads is already checked for writes. No terminator/region bookkeeping is needed for that.

**On extending the linear `MemEffectsCache` walk across blocks.** I think this is where the "just continue to `toOp`" model runs into a soundness problem, because the bug is cross-block and once we leave a single block we have diamonds and back edges rather than a linear path. Concretely:

```mlir
%0 = memref.load %m[%i]     // fromOp (preheader)
cf.br ^bb1
^bb1:
  %1 = memref.load %m[%i]   // toOp
  memref.store %x, %m[%i]   // write is AFTER toOp
  cf.cond_br %c, ^bb1, ^bb2 // back edge
```

`%1` sees a different value on the second iteration, so it must not be replaced by `%0`. A walk that stops as soon as it reaches `%1` never observes the store (it's after `%1`) — the store only reaches a later `%1` through the back edge — so it would CSE incorrectly. That's why the PR instead computes the block slice reachable from the first read that can also reach the second, and bails when that slice contains a cycle (the `cross_block_loop_write_blocks` test covers exactly this). If there's a linear-walk formulation that stays sound under back edges, I'm very open to it — I may just be missing it.

**On the cache.** Fair point that the cross-block path doesn't reuse `MemEffectsCache`, so repeated cross-block queries aren't memoized. I can look at extending the cache to cover the cross-block scan if we keep this in CSE.


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