[Mlir-commits] [mlir] [mlir][VectorToLLVM] emit `inbounds|nuw` GEP flags when lowering `vector.load/store` (PR #202118)

[Andrzej Warzyński]

================
@@ -256,10 +256,19 @@ class VectorLoadStoreConversion : public ConvertOpToLLVMPattern<LoadOrStoreOp> {
                                          "could not resolve alignment");
 
     // Resolve address.
+    // Per vector.load/store spec, indices must be in-bounds (0 <= idx <
+    // dim_size). Emit inbounds|nuw so LLVM can apply no-wrap optimizations on
----------------
banach-space wrote:

That's a great summary, thank you!

### Re inbounds

> inbounds (and the implied nusw/nsw on the index arithmetic) reduce to a single assumption: the addressed element lies within the allocation.

What about (from https://llvm.org/docs/LangRef.html#id241):

> During the successive addition of offsets to the address, the resulting pointer must remain in bounds of the allocated object at each step.

? How does it affect `Vector` lowerings?

Also from the same LangRef docs:

> If any of the rules are violated, the result value is a [poison value (https://llvm.org/docs/LangRef.html#poisonvalues)

This means that `memref.load` [docs](https://mlir.llvm.org/docs/Dialects/MemRef/#memrefload-memrefloadop) are incorrect:

> Lowerings of memref.load may emit attributes, e.g. inbouds + nuw when converting to LLVM’s llvm.getelementptr, that would cause undefined behavior if indices are out of bounds or if computing the offset in the memref would cause signed overflow of the index type.

no? Or is the address that's `poison`?

Finally, from your reply:

> Note the vector.load spec is careful to distinguish two failure modes: OOB elements have undefined values, and only the alignment clause triggers immediate undefined behavior, but the lowering to a plain load doesn't preserve that distinction

Good point that's worth documenting.

> In LLVM IR this is not "the lane 7 is undefined": for the Pointer Aliasing Rules it is simply UB (if I understood correctly).

That's consistent with my reading of the docs.

### Re nuw

> Correct me if I'm wrong, but we can land inbounds while violating nuw if you get there by walking backwards (also in memref maybe). E.g., a flipped view (from [0, 1000) -> [900, 1000)):

Great point - we shouldn't add `nuw` when strides are negative. That restriction should apply for both `MemRef` and `Vector`. I suggest adding that restriction in a dedicated PR.

> That's to say, it's not correct to put a priori nuw here. But then the same goes for memref, am I missing something?

Indeed.

### Re the docs

> I think that we should clarify the vector.load/vector.store index semantics. E.g., requiring the start address to be in-bounds, and stating the non-negativity conditions under which lowerings may emit inbounds/nuw, mirroring the memref.load/memref.store wording.

Strongly agree and I would happily review your contributions in this direction. 

However, please note that there's a difference between "vector.load semantics" and "vector.load semantics when lowering to LLVM". We don't want `Vector` docs to assume that LLVM is the only target, but IMO it's OK to document the behaviour specific to LLVM.

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