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

[Andrzej Warzyński]

banach-space wrote:

Hi @FedericoBruzzone, thank you for the updates and for all the replies.

I think there is still a mismatch between what I intended in my earlier comments and what the current implementation does, so let me clarify.

> The right thing to do is make the flags (`inbounds` and `nneg`) optional and let the specific lowering decide whether to include them or not (or update the docs accordingly).

My concern is specifically with adding these as attributes on `vector.load` / `vector.store` themselves. I do not think we should expose `inbounds` / `nneg` at the Vector dialect level unless there is a clear dialect-level semantic need for them.

In particular, this seems to push LLVM-specific lowering details into the Vector dialect and creates extra burden for producers/lowerings that generate `vector.load` / `vector.store`: they now need to reason about, preserve, and propagate these attributes even though the intended consumer is the LLVM lowering.

What I was suggesting instead is to keep the Vector ops unchanged and restore the original direction of this patch: emit the appropriate `inbounds` / `nuw` flags when lowering to LLVM, assuming the LLVM lowering semantics/docs justify doing so. 

So my preference would be:

1. Do not add `inbounds` / `nneg` attributes to `vector.load` / `vector.store`.
2. Restore the previous LLVM lowering behavior from this patch where the GEP flags are emitted by the lowering itself.
3. If needed, update the Vector-to-LLVM documentation to make the lowering assumptions explicit.
4. Update the Vector dialect docs to require "this has 0 <= idx < dim for all dimensions", at least when lowering to LLVM.

If there are cases when adding `inbounds` / `nneg` would violate some assumptions, we can add flags to Vector-To-LLVM to disable those flags conditionally. Does this make sense? 

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