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

[Federico Bruzzone]

FedericoBruzzone wrote:

> 1. I could've sworn we didn't allow negative strides in memref, so `nuw` and `inbounds` on `memref.load` is correct

Unfortunately, I think that this is not true, but please correct me if I'm wrong 🙏
My conclusions are guided by the following two tests:
https://github.com/llvm/llvm-project/blob/db210c5e33643a8154329a607b5b6dcd419271df/mlir/test/Dialect/MemRef/canonicalize.mlir#L1290-L1305
https://github.com/llvm/llvm-project/blob/db210c5e33643a8154329a607b5b6dcd419271df/mlir/test/Conversion/MemRefToLLVM/expand-then-convert-to-llvm.mlir#L356-L376
If we confirm, I'd  be glad to send a subsequent patch for this (with the docs fixed).


> 2. I still think that putting flags on `vector.load` for "this has 0 <= idx < dim for all dimensions" and "none of these indices are negative" as extra assumptions you can encode is the right move - it means that if you don't set these flags, we just don't say `inbounds` and `nuw`

I really like this approach! But shouldn't this behavior also apply to memref?
What I don't understand is whether it makes sense to have two different behaviors for `memref.load` and `vector.load`.
Additionally, `nneg` is solely for indices, it's not imply `nuw`. Basically, "`nuw`" == "`nneg` with non-negative stride". Should we cover this thing?


> 3. Last I checked, LLVM _doesn't_ produce poison if you index outside of an object with a non-`inbounds` GEP, hence the flag? Like, I'm pretty sure you're allowed to do
> 
> ```llvm
> ;; %x points to a [2 x i32]
> ;; %y is a valid pointer to the end of %x, as in .end() for array iterators in C++
> %y = getelementptr i32, ptr %x, i64 3
> ;; this is probably UB, or at least target-defined
> %l = load i32, ptr %y
> ```
> 
> So you only get immediate `poison` if you mark the GEP `inbounds`.
> 
> (Now, for buffer fat pointers, everything works out. If you `inbounds` your pointer, you're promising you're in the logical object pointer pointed to by the resource. If you don't, you're off the edge of the object, and target-specific documentation says that going off the end means reading 0 / dropping writes (maybe not super-clearly but), which is a perfectly correct refinement of LLVM UB.

Exactly. You're right. Poison values ​​only occur when an attribute is violated (approximately). Since the GEP does't acces memory your GEP is free from poisons.

To answer this question (UB or target-defined?), which does not seem to be clarified by the llvm.load documentation alone
```llvm
%l = load i32, ptr %y
```
the [Pointer Aliasing Rules](https://llvm.org/docs/LangRef.html#pointer-aliasing-rules) say: "any memory access must be done through a pointer value associated with an address range of the memory access, otherwise the behavior is undefined", hence I'd say UB confirmed.

> Also, the OOB tail is LLVM-level UB, but, again, in graphics contexts or other such cases, you can absolutely have _defined_ behavior for such a tail. It's perhaps more correct to say that OOB tails, OOB start indices, etc. have _implementation-defined_ behavior at the MLIR level. If you want to explicitly exclude the possibility, you should put a flag on.


That's true as well. I really like to leave the choice of including attributes or not to the specific lowering.


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