[Mlir-commits] [mlir] [mlir][vector] Add opt-in `inbounds`/`nneg` flags to `vector.load`/`vector.store` (PR #202118)

[Andrzej Warzyński]

banach-space wrote:

> I don't know how to answer this question due to lack of experience

No problem, there's a crazy amount of nuance here! Non-unit trailing strides are already rejected by the verifier, see for example this test:
https://github.com/llvm/llvm-project/blob/f6e4e71fcdbff98cb060f87695c75f00b43bf918/mlir/test/Dialect/Vector/invalid.mlir?plain=1#L2103-L2108

The verifier logic is implemented here: https://github.com/llvm/llvm-project/blob/56eb89eddd921400e82d958aa32c1ed54b154362/mlir/lib/Dialect/Vector/IR/VectorOps.cpp?plain=1#L5908-L5909

Given the confusion in this PR, I suggest adding an assert in the lowering code to further document the restriction. Leaving this as a TODO for @FedericoBruzzone :)

---

> I'm quite opposed to encoding properties of your program in IR pass options.

I don't think that objection should block this PR.

The newly added flag defaults to `false`, so the existing behaviour is preserved. Users that can guarantee the stronger precondition can opt into the corresponding lowering path; users that cannot, or do not want to, get exactly the current behaviour.

Whether these facts should eventually be represented directly in the IR is a separate design discussion. It should not be a prerequisite for this PR.

> * Re in-bounds being useful MLIR-level, I'm going to point at, for example https://github.com/llvm/llvm-project/blob/9c6a5f064611b12fd38f7282c0a3b600eb30950d/mlir/lib/Dialect/Vector/Transforms/IndexedAccessOpInterfaceImpl.cpp#L179
>    being a method I've added _specifically_ to capture whether or not a vector.load is starting in-bounds
> * Re "inbounds isn't used", I point to things like https://github.com/llvm/llvm-project/blob/9c6a5f064611b12fd38f7282c0a3b600eb30950d/mlir/lib/Dialect/Vector/Transforms/IndexedAccessOpInterfaceImpl.cpp#L179

These examples point to TODO-level support. That is not enough to make this PR depend on those attributes.

`Vector` dialect does not currently maintain this information. Making lowering rely on such attributes would require auditing and updating the relevant `Vector` transformations to preserve them correctly. That is a much larger scope than this PR, and I don't think it is reasonable to require it here.

So I don't think this should block the current approach. If we want MLIR-level attributes to carry this information reliably, that should be done as a separate follow-up effort.

---

> BTW, to provide a concrete view of the patch I'll submit consider the following pipeline:

As in, you are proposing a patch that will update `memref.store` lowering when strides are negative? SGTM

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