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

[Federico Bruzzone]

FedericoBruzzone wrote:

> No, I don't think so. I was worried about loads that read more than one SIMD-register worth of data, e.g. (for NEON):
> %0 = vector.load %src[%i0] : memref<?xf32>, vector<8xf32>
> Loads like that need to be "decomposed", but that will happen in LLVM. We don't need to worry about it at the MLIR-> LLVM boundary. All good!

It's a pleasure for me. We needed to investigate these things a little. I'd say it makes sense to me too. Good :D

> I am against this change.

And you're right to be against it :D In fact, in the next comment, I realized it wasn't correct.
The right thing to do is make the flags (inbound and nneg) optional and let the specific lowering decide whether to include them or not (or update the docs accordingly). This is already done in this PR (see the PR body for details). I hope it meets your expectations. If it's not, I'm glad to to make changes <3

> +1 for forbidding them, at least in the context of lowering to LLVM.

It would make sense to align MemRef (also the docs) in the direction of this PR, but only for `nuws/nneg`, preventing UB on the LLVM lowering (?)
We need to understand whether we want to keep `The indices must be in-bounds: 0 <= idx < dim_size.` for MemRef, but (i) we can talk about it in the next PR, and (ii) unfortunately I haven't thought long enough to figure out whether it makes sense to remove it or not. **Do you have any ideas yet?** 
NOTE: It's trivial, but obviously if we keep the indexes inbound (for memref) then `memref.load` and `vector.load` would be misaligned in this direction.

> Do people lower via LLVM in those contexts?

I was wondering the same thing, and from what the internet/AI tells me, it seems so. But would it be wrong to assume it's always this way?

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