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

[Federico Bruzzone]

FedericoBruzzone wrote:

Before proceeding with @banach-space 's valuable suggestions, I'd like to clarify the situation for a moment. While the LLVM docs is precious, it's a bit confusing. But if read carefully I think it answers all our doubts.

First of all, thanks again and sorry for the delay in responding :D  
I'll try to close each sub-thread.

## Re inbounds / "remain in bounds at each step". 

> From @banach-space :
> 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?

`getStridedElementPtr` linearizes all indices into a single offset and emits exactly **one** `getelementptr`, so the "at each step" rule collapses to a single step: the only requirement is that the start address be in bounds of the allocated object, am I missing something?
https://github.com/llvm/llvm-project/blob/cf004c900376a60386d8cc7c643ea07cd61217e9/mlir/lib/Conversion/LLVMCommon/Pattern.cpp#L626-L649
It does not constrain the width of the subsequent vector load. So the documented ["explicit out-of-bound"](https://mlir.llvm.org/docs/Dialects/Vector/#vectorload-vectorloadop) example `vector.load %m[%c0] : memref<7xf32>, vector<8xf32>` is unaffected: `%c0` is in bounds (and [GEP "with all-zero indices is always considered to be inbounds"](https://llvm.org/docs/LangRef.html#getelementptr-instruction) by definition). The OOB tail is the `llvm.load`'s problem and is already UB today, with or without these flags. That is, `inbounds` doesn't remove any behavior the LLVM lowering actually provided, every case it turns into poison was already UB at the wide load.
**Conclusion**:
1. I'd add `inbounds` to `vector.load` as also suggested by @krzysz00. Sorry for the dubt, but I don't understand where to put `nneg` :( Could I have a little more context on this?

## Re poison vs UB.

> From @banach-space:
> 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?

You're right that the address (the GEP result) that is poison value (LangRef: "If any of the rules are violated, the result value is a poison value"). But the immediately following `llvm.load/store` dereferences that poison pointer, and "immediate undefined behavior occurs if a poison value is used as ... the pointer operand of a load, store ...". There's even the exact `store i32 0, ptr %poison ; Undefined behavior example`). 
```llvm
  %poison = sub nuw i32 0, 1           ; Results in a poison value.
  %poison2 = sub i32 poison, 1         ; Also results in a poison value.
  %still_poison = and i32 %poison, 0   ; 0, but also poison.
  %poison_yet_again = getelementptr i32, ptr @h, i32 %still_poison
  store i32 0, ptr %poison_yet_again   ; Undefined behavior due to
                                       ; store to poison.

  store i32 %poison, ptr @g            ; Poison value stored to memory.
  %poison3 = load i32, ptr @g          ; Poison value loaded back from memory.

  %poison4 = load i16, ptr @g          ; Returns a poison value.
  %poison5 = load i64, ptr @g          ; Returns a poison value.

  %cmp = icmp slt i32 %poison, 0       ; Returns a poison value.
  br i1 %cmp, label %end, label %end   ; undefined behavior

```
So the `memref.load` [docs](https://mlir.llvm.org/docs/Dialects/MemRef/#memrefload-memrefloadop) aren't wrong in their conclusion (UB), they just compress the intermediate "GEP yields poison -> deref of poison is UB" step. 
**Conclusions:** 
1. I'll send a small docs PR to make that precise.



## Re nuw

Fully agreed. `nuw` is unsound with negative strides, and `memref.load` has the same latent issue today since it also emits `inbounds|nuw` unconditionally. 

**Conclusions:**
1. Do we agree on "make the logic that lowers to LLVM reject MemRef(s) with negative strides (to avoid violating nuw)"? (It perfectly makes sense to me, I'll submit a PR for this)
2. How do we handle this for `vector`? Since we want to keep `vector.load %mem[-1] : vector<4xf32>, memref<8xf32, #spirv.relevant_buffer_kind>` alive as @krzysz00 pointed out, we should than have a specific semantics (and docs as for memref) for LLVM lowering (something like: "start index in-bounds; OOB start index is UB under that lowering to LLVM") as @banach-space suggest me (there's a difference between "vector.load semantics" and "vector.load semantics when lowering to LLVM"). In short: while `memref` maintains "The indices must be in-bounds: 0 <= idx < dim_size.", for vector this should not be guaranteed. Is this ok :D?

Now, we have two paths for this PR:
1. Keep `inbounds|nuw` as `memref.load`, and the guard on negative strides goes into a dedicated PR that fixes memref and vector together.
2. Emit only inbounds in this patch and reintroduce `nuw` (with guard) into the unification PR. 

## Re abusing of UB

> From @krzysz00  
> AMDGPU's buffer address spaces still have a meaningful use for gep inbounds vs not - the former means you're in the bounds of the buffer, the latter means you might be pointed at the 0s.

Although I have to admit that I'm really fascinated by this thing *-*
As @fabianmcg,  I think that even though LLVM may not know the size at compile time this is an abuse on UB.

> From @fabianmcg  
> If we take that an out of bounds ptr as poison, then the result of the load is UB. Consequently, no, it's not valid unless one stablishes that there are no out-of-bounds (wrt LLVM semantics) in that address space. Otherwise, the optimizer is free to just remove those instructions.

That's exactly the problem. In accordance with the LLVM semantics,`opt` can just treat the code as dead.

--- 

In addition to our decisions I'll drop `vectorize_inbounds_llvmopt.mlir` :)


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