[Mlir-commits] [mlir] [mlir][vector] Clarify the semantics of gather/scatter indexing (nfc) (PR #181357)

[Andrzej Warzyński]

================
@@ -2082,23 +2082,27 @@ def Vector_GatherOp :
     3-D and the result is 2-D:
 
     ```mlir
-    func.func @gather_3D_to_2D(
-        %base: memref<?x10x?xf32>, %ofs_0: index, %ofs_1: index, %ofs_2: index,
-        %indices: vector<2x3xi32>, %mask: vector<2x3xi1>,
-        %fall_thru: vector<2x3xf32>) -> vector<2x3xf32> {
-            %result = vector.gather %base[%ofs_0, %ofs_1, %ofs_2]
-                                   [%indices], %mask, %fall_thru : [...]
-            return %result : vector<2x3xf32>
+      %base: memref<?x10x?xf32>, %ofs_0: index, %ofs_1: index, %ofs_2: index,
+      %indices: vector<2x3xi32>, %mask: vector<2x3xi1>,
+      %fall_thru: vector<2x3xf32>) -> vector<2x3xf32> {
+          %result = vector.gather %base[%ofs_0, %ofs_1, %ofs_2]
+                                 [%indices], %mask, %fall_thru : [...]
     }
     ```
 
     The indexing semantics are then,
 
     ```
-    result[i,j] := if mask[i,j] then base[i0, i1, i2 + indices[i,j]]
+    result[i,j] := if mask[i,j] then base[%ofs_0, %ofs_1, %ofs_2 + indices[i,j]]
                    else pass_thru[i,j]
     ```
-    The index into `base` only varies in the innermost ((k-1)-th) dimension.
+    Note, `indices` are element offsets - they are expressed in units of
+    elements (not bytes). Each element in `indices` represents a displacement
+    in units of elements from the starting element, i.e. `%base[%ofs_0, %ofs_1,
+    %ofs_2]` for the example above. Importantly, for MemRefs, `indices` are
+    %interpreted assuming an identity (contiguous) MemRef
+    layout and do not account for non-identity strides.
----------------
banach-space wrote:

> will read OOB?

If your indices are > 0 and mask is "all true", then OOB, yes.  At least based on the available context. 

I guess that you could consider `%value` to be a view into a larger source MemRef and in the context of that MemRef it would possibly be OK? I think that we should stick with the "local" view though.

> Test case for the inconsistency https://github.com/llvm/llvm-project/pull/187071#issuecomment-4077846358

Thanks! I created https://github.com/llvm/llvm-project/issues/187215 so that we have one central place to discuss that and so that there's a repro that people can use.

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