[Mlir-commits] [mlir] [mlir][vector][docs] Document indexed vs. non-indexed arguments (PR #130141)

[Quinn Dawkins]

================
@@ -257,6 +257,96 @@ expressing `vector`s in the IR directly and simple pattern-rewrites.
 [EDSC](https://github.com/llvm/llvm-project/blob/main/mlir/docs/EDSC.md)s
 provide a simple way of driving such a notional language directly in C++.
 
+### Taxonomy for "Read"/"Write" Operations
+
+Below is a list of vector dialect operations that move values from an abstract
+**source** to an abstract **destination**, i.e. "read"/"write" operations:
+
+* `vector.load`, `vector.store`, `vector.transfer_read`,
+  `vector.transfer_write`, `vector.gather`, `vector.scatter`,
+  `vector.compressstore`, `vector.expandload`, `vector.maskedload`,
+  `vector.maskedstore`, `vector.extract`, `vector.insert`,
+  `vector.scalable_extract`, `vector.scalable_insert`,
+  `vector.extract_strided_slice`, `vector.insert_strided_slice`.
+
+#### Current Naming in Vector Dialect
+| **Vector Dialect Op**          | **Operand Names**        | **Operand Types**             | **Result Name**  | **Result Type**      |
+|--------------------------------|--------------------------|-------------------------------|------------------|----------------------|
+| `vector.load`                  | `base`                   | `memref`                      | `result`         | `vector`             |
+| `vector.store`                 | `valueToStore`, `base`   | `vector`, `memref`            | -                | -                    |
+| `vector.transfer_read`         | `source`                 | `memref` / `tensor`           | `vector`         | `vector`             |
+| `vector.transfer_write`        | `vector`, `source`       | `vector`, `memref`/ `tensor`  | `result`         | `vector`             |
+| `vector.gather`                | `base`                   | `memref`                      | `result`         | `vector`             |
+| `vector.scatter`               | `valueToStore`, `base`   | `vector`, `memref`            | -                | -                    |
+| `vector.expandload`            | `base`                   | `memref`                      | `result`         | `vector`             |
+| `vector.compressstore`         | `valueToStore`,`base`    | `vector`, `memref`            | -                | -                    |
+| `vector.maskedload`            | `base`                   | `memref`                      | `result`         | `vector`             |
+| `vector.maskedstore`           | `valueToStore`, `base`   | `vector`, `memref`            | -                | -                    |
+| `vector.extract`               | `vector`                 | `vector`                      | `result`         | `scalar` / `vector`  |
+| `vector.insert`                | `source`, `dest`         | `scalar` / `vector`, `vector` | `result`         | `vector`             |
+| `vector.scalable_extract`      | `source`                 | `vector`                      | `res`            | `scalar` / `vector`  |
+| `vector.scalable_insert`       | `source`, `dest`         | `scalar` / `vector`, `vector` | `res`            | `vector`             |
+| `vector.extract_strided_slice` | `vector`                 | `vector`                      | (missing name)   | `vector`             |
+| `vector.insert_strided_slice`  | `source`, `dest`         | `vector`                      | `res`            | `vector`             |
+
+Note that "read" operations take one operand ("from"), whereas "write"
+operations require two ("value-to-store" and "to").
+
+### Observations
+Each "read" operation has a "from" argument, while each "write" operation has a
+"to" and a "value-to-store" operand. However, the naming conventions are
+**inconsistent**, making it difficult to extract common patterns or determine
+operand roles. Here are some inconsistencies:
----------------
qedawkins wrote:

I agree with Kunwar, transient documentation reads like a complaint about bikeshedding on naming.

I suspect there is a more practical reason for most of the naming here and that is to ease templated patterns. One of the most glaring cases is `getSource` for `transfer_write`. This appears unusual until you realize this makes it very easy to write templated patterns/helpers that support both transfer ops. This is also the same reason the input to `transfer_write` is `vector`, the same name as the result of `transfer_read` (the other glaring example in the above table). The same reasoning applies to `vector.load`/`vector.store`.

If the naming is a problem, I would prefer we just fix it. Justified NFC renamings don't create all that much churn IMHO. My suggestions would be:

 - Rename `source` on transfer_read/write to `base`
 - Rename `vector` on transfer_write to `valueToStore` and `vector` on `transfer_read` to `result`
 - Add either extraClassDeclarations or a method to `VectorTransferOpInterface` called `getTransferVector` and `getTransferSource` for use with shared patterns
 - Rename `vector` on `vector.extract` to `source`
 - Rename `res` to `result` on all other applicable ops

Thanks for the great summary of the current state! It makes it very obvious to me where we can improve.

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