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

[Andrzej Warzyński]

banach-space wrote:

> For negative offsets, yeah, I've seen that sort of thing. `%idx = select %isOOB, %realIdx, %cBecomes_uint32_max_bytes; vector.store %mem[%idx] : memref<NxT, #amdgpu.address_space<raw_fat_buffer_pointer>>` is a construct that comes up again, though sometimes that constant is just `%idx = select %isOOB, %readIdx, %dimOfMem`

TBH, it's not clear to me how that is related to `ConvertVectorToLLVM. It would be helpful if a specific use case was presented that we can discuss and see whether negative offsets are truly required. That's a separate discussion though, for a different PR.

> I'm partly arguing for these flags because there are MLIR-level transformations that are enabled by knowing your load/store indices are in-bounds, which, unlike for memref.load, was never a documented property of vector.load, so we need to annotate for it

This patch is not adding such transformations hence it should not be adding those extra attributes. As a co-maintainer of `Vector`, I am strongly against adding functionality that is not used.

> Also, re in_bounds on transfer_read, I'm pretty sure there are a bunch of patterns that will check that to see if they need to decompose the transfer_read into a bunch of smaller operations or add guards or not.

Without specific examples, this is very speculative and also does not match my experience.

> To reiterate, vector.load, unlike memref.load (and analogously for store) have historically allowed for implementation-defined behavior for negative or out of bounds indices

I don't recall folks agreeing to out-of-bounds indices and/or that being documented anywhere.

---

We are digressing quite a bit and we are also being held back by ambiguities in LLVM's LangRef. This is not time and place to resolve those.

My recommendation remains the same, see [here](https://github.com/llvm/llvm-project/pull/202118#issuecomment-4708500204). In addition, to avoid contention, make adding `inbounds / nneg` an opt-in for `ConvertVectorToLLVM` (this way we can skip updating the docs).

Thank you again for looking into this!

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