[Mlir-commits] [mlir] 4e4226d - [mlir][vector] Clarify OOB semantics for `gather` and `scatter`

[Jakub Kuderski]

Author: Jakub Kuderski
Date: 2023-03-13T13:58:12-04:00
New Revision: 4e4226dc362f851ab57b8169041b722128e23a66

URL: https://github.com/llvm/llvm-project/commit/4e4226dc362f851ab57b8169041b722128e23a66
DIFF: https://github.com/llvm/llvm-project/commit/4e4226dc362f851ab57b8169041b722128e23a66.diff

LOG: [mlir][vector] Clarify OOB semantics for `gather` and `scatter`

Reword the vector gather and scatter op description to make it
well-defined to have out-of-bounds indices when the corresponding mask
bits are false.

Update the code sample to avoid relying on C UB semantics to provide
informal semantics for vector.gather.

This change should be consistent with the existing interpretation of the
semantics in the codebase.

Issue: https://github.com/llvm/llvm-project/issues/60905

Reviewed By: dcaballe

Differential Revision: https://reviews.llvm.org/D145824

Added: 
    

Modified: 
    mlir/include/mlir/Dialect/Vector/IR/VectorOps.td

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index 6a5913e512388..b9815664dfed0 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -1857,21 +1857,27 @@ def Vector_GatherOp :
 
   let summary = [{
     gathers elements from memory or ranked tensor into a vector as defined by an
-    index vector and mask
+    index vector and a mask vector
   }];
 
   let description = [{
-    The gather operation gathers elements from memory or ranked tensor into a
-    n-D vector as defined by a base with indices and an additional n-D index
-    vector (each index is a 1-D offset on the base), but only if the
-    corresponding bit is set in a n-D mask vector. Otherwise, the element is
-    taken from a n-D pass-through vector. Informally the semantics are:
-    ```
-    result[0] := mask[0] ? base[index[0]] : pass_thru[0]
-    result[1] := mask[1] ? base[index[1]] : pass_thru[1]
+    The gather operation returns an n-D vector whose elements are either loaded
+    from memory or ranked tensor, or taken from a pass-through vector, depending
+    on the values of an n-D mask vector.
+    If a mask bit is set, the corresponding result element is defined by the base
+    with indices and the n-D index vector (each index is a 1-D offset on the base).
+    Otherwise, the corresponding element is taken from the n-D pass-through vector.
+    Informally the semantics are:
+    ```
+    result[0] := if mask[0] then base[index[0]] else pass_thru[0]
+    result[1] := if mask[1] then base[index[1]] else pass_thru[1]
     etc.
     ```
-    The vector dialect leaves out-of-bounds behavior undefined.
+
+    If a mask bit is set and the corresponding index is out-of-bounds for the
+    given base, the behavior is undefined. If a mask bit is not set, the value
+    comes from the pass-through vector regardless of the index, and the index is
+    allowed to be out-of-bounds.
 
     The gather operation can be used directly where applicable, or can be used
     during progressively lowering to bring other memory operations closer to
@@ -1921,10 +1927,13 @@ def Vector_ScatterOp :
                VectorOfRankAndType<[1], [I1]>:$mask,
                VectorOfRank<[1]>:$valueToStore)> {
 
-  let summary = "scatters elements from a vector into memory as defined by an index vector and mask";
+  let summary = [{
+    scatters elements from a vector into memory as defined by an index vector
+    and a mask vector
+  }];
 
   let description = [{
-    The scatter operation scatters elements from a 1-D vector into memory as
+    The scatter operation stores elements from a 1-D vector into memory as
     defined by a base with indices and an additional 1-D index vector, but
     only if the corresponding bit in a 1-D mask vector is set. Otherwise, no
     action is taken for that element. Informally the semantics are:
@@ -1933,10 +1942,16 @@ def Vector_ScatterOp :
     if (mask[1]) base[index[1]] = value[1]
     etc.
     ```
-    The vector dialect leaves out-of-bounds and repeated index behavior
-    undefined. Underlying implementations may enforce strict sequential
-    semantics for the latter, though.
-    TODO: enforce the latter always?
+
+    If a mask bit is set and the corresponding index is out-of-bounds for the
+    given base, the behavior is undefined. If a mask bit is not set, no value
+    is stored regardless of the index, and the index is allowed to be
+    out-of-bounds.
+
+    If the index vector contains two or more duplicate indices, the behavior is
+    undefined. Underlying implementation may enforce strict sequential
+    semantics.
+    TODO: always enforce strict sequential semantics?
 
     The scatter operation can be used directly where applicable, or can be used
     during progressively lowering to bring other memory operations closer to