[Mlir-commits] [mlir] [mlir][affine] Fix replaceAllMemRefUsesWith failing on memref.load/store (PR #217833)

[Alessandro Potenza]

https://github.com/alepot55 commented:

Read, not built, so nothing below is an execution result.

**The clearest argument for this patch is that the file already contradicted itself**, and the description does not say so. `Utils.cpp` has the same question answered twice:

```cpp
// line 1159, the single-op overload
if (!isDereferencingOp(op)) { ... }

// line 1360, the collector this PR changes
if (!isa<AffineMapAccessInterface>(*user)) { ... }
```

with

```cpp
static bool isDereferencingOp(Operation *op) {
  return isa<AffineMapAccessInterface, memref::LoadOp, memref::StoreOp>(op);
}
```

The collector hands every op it accepts to that single-op overload, so the two were disagreeing about the same three op classes, and the stricter of the two ran first. That is a much stronger case than "memref.load/store are dereferencing ops", because it does not require the reader to agree with a definition, only to notice that one function rejects what the function it delegates to accepts.

**The obvious follow-up question is whether the two look-alikes should change too, and I think the answer is no.** Two other places use the same predicate:

- `Transforms/LoopFusion.cpp:210`, deciding whether a memref is used by a non-dereferencing op.
- `Analysis/Utils.cpp:509`, whose comment is *"Return true if any use of 'memref' does not deference it in an affine way"*.

Those are asking whether the **affine dependence machinery can reason about the access**, not whether the use can be rewritten. A `memref.load` is a real dereference but not an affine one, so counting it as opaque there is the conservative and correct answer. Worth a line in the description saying so, because it is the first thing a reviewer will wonder and it currently reads as though the predicate is simply wrong everywhere.

**One behavioural note that is not in the description.** `memref.store` carries `MemRefsNormalizable`. So before this patch, with `allowNonDereferencingOps` set, a `memref.store` user fell into the non-dereferencing branch, passed the trait check, was inserted into `opsToReplace`, and then the single-op overload treated it as dereferencing anyway and rewrote its indices. The result happened to be right, by a route nobody intended. After the patch both agree up front. That is a second thing this fixes, and it is worth one sentence.

The test is well chosen: `memref.store` alongside `affine.store` on the same memref is exactly the shape that used to make the whole replacement bail and leave the fast buffer uninitialised.

@bondhugula, @arnab-polymage: this has been waiting since 21 August.

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