[mlir][Interfaces][NFC] Update doc of ViewLikeOpInterface parser/printer handlers

[dcaballe]

This PR addresses part of the feedback provided in #115808.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Diego Caballero (dcaballe)

Changes

This PR addresses part of the feedback provided in #115808.

---

Full diff: https://github.com/llvm/llvm-project/pull/122555.diff

1 Files Affected:

• (modified) mlir/include/mlir/Interfaces/ViewLikeInterface.h (+58-40)

diff --git a/mlir/include/mlir/Interfaces/ViewLikeInterface.h b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
index d6479143a0a50b..f374be3de9a2ad 100644
--- a/mlir/include/mlir/Interfaces/ViewLikeInterface.h
+++ b/mlir/include/mlir/Interfaces/ViewLikeInterface.h
@@ -86,24 +86,35 @@ class OpWithOffsetSizesAndStridesConstantArgumentFolder final
   }
 };
 
-/// Printer hook for custom directive in assemblyFormat.
+/// Printer hooks for custom directive in assemblyFormat.
 ///
 ///   custom<DynamicIndexList>($values, $integers)
 ///   custom<DynamicIndexList>($values, $integers, type($values))
 ///
-/// where `values` is of ODS type `Variadic<*>` and `integers` is of ODS
-/// type `I64ArrayAttr`. Prints a list with either (1) the static integer value
-/// in `integers` is `kDynamic` or (2) the next value otherwise. If `valueTypes`
-/// is non-empty, it is expected to contain as many elements as `values`
-/// indicating their types. This allows idiomatic printing of mixed value and
-/// integer attributes in a list. E.g.
-/// `[%arg0 : index, 7, 42, %arg42 : i32]`.
+/// where `values` is of ODS type `Variadic<*>` and `integers` is of ODS type
+/// `I64ArrayAttr`. Print a list where each element is either:
+///    1. the static integer value in `integers`, if it's not `kDynamic` or,
+///    2. the next value in `values` with its corresponding type, otherwise.
+///
+/// The type for integer elements is `i64` by default and not printed.
+///
+/// If `valueTypes` is provided, verify that each type matches the type of the
+/// corresponding `values`. Providing `valueTypes` is redundant for printing so
+/// we don't use `valueTypes` after the verification.
+///
+/// Integer indices can also be scalable, denoted with square brackets (e.g.,
+/// "[2, [4], 8]"). For each value in `integers`, the corresponding `bool` in
+/// `scalables` encodes whether it's a scalable index. If `scalables` is empty
+/// then assume that all indices are non-scalable.
+///
+/// Examples:
+///   * For `integers = [kDynamic, 7, 42, kDynamic]`,
+///         `values = [%arg0, %arg42]`,Using the default type for integers and
+///         `valueTypes` for `values`:
+///       `[%arg0 : index, 7, 42, %arg42 : i32]`
+///   * Using `scalables`:
+///       `[2, [4], 8]`
 ///
-/// Indices can be scalable. For example, "4" in "[2, [4], 8]" is scalable.
-/// This notation is similar to how scalable dims are marked when defining
-/// Vectors. For each value in `integers`, the corresponding `bool` in
-/// `scalables` encodes whether it's a scalable index. If `scalableVals` is
-/// empty then assume that all indices are non-scalable.
 void printDynamicIndexList(
     OpAsmPrinter &printer, Operation *op, OperandRange values,
     ArrayRef<int64_t> integers, ArrayRef<bool> scalables,
@@ -113,35 +124,43 @@ inline void printDynamicIndexList(
     OpAsmPrinter &printer, Operation *op, OperandRange values,
     ArrayRef<int64_t> integers, TypeRange valueTypes = TypeRange(),
     AsmParser::Delimiter delimiter = AsmParser::Delimiter::Square) {
-  return printDynamicIndexList(printer, op, values, integers, {}, valueTypes,
-                               delimiter);
+  return printDynamicIndexList(printer, op, values, integers, /*scalables=*/{},
+                               valueTypes, delimiter);
 }
 
-/// Parser hook for custom directive in assemblyFormat.
+/// Parser hooks for custom directive in assemblyFormat.
 ///
 ///   custom<DynamicIndexList>($values, $integers)
 ///   custom<DynamicIndexList>($values, $integers, type($values))
 ///
 /// where `values` is of ODS type `Variadic<*>` and `integers` is of ODS
-/// type `I64ArrayAttr`. Parse a mixed list with either (1) static integer
-/// values or (2) SSA values. Fill `integers` with the integer ArrayAttr, where
-/// `kDynamic` encodes the position of SSA values. Add the parsed SSA values
-/// to `values` in-order. If `valueTypes` is non-null, fill it with types
-/// corresponding to values; otherwise the caller must handle the types.
+/// type `I64ArrayAttr`. Parse a mixed list where each element is either a
+/// static integer or an SSA value. Fill `integers` with the integer ArrayAttr,
+/// where `kDynamic` encodes the position of SSA values. Add the parsed SSA
+/// values to `values` in-order.
 ///
-/// E.g. after parsing "[%arg0 : index, 7, 42, %arg42 : i32]":
-///   1. `result` is filled with the i64 ArrayAttr "[`kDynamic`, 7, 42,
-///   `kDynamic`]"
-///   2. `ssa` is filled with "[%arg0, %arg1]".
+/// If `valueTypes` is provided, fill it with the types corresponding to each
+/// value in `values`. Otherwise, the caller must handle the types.
 ///
-/// Indices can be scalable. For example, "4" in "[2, [4], 8]" is scalable.
-/// This notation is similar to how scalable dims are marked when defining
-/// Vectors. For each value in `integers`, the corresponding `bool` in
-/// `scalableVals` encodes whether it's a scalable index.
+/// Integer indices can also be scalable, denoted by the square bracket (e.g.,
+/// "[2, [4], 8]"). For each value in `integers`, the corresponding `bool` in
+/// `scalables` encodes whether it's a scalable index.
+///
+/// Examples:
+///   * After parsing "[%arg0 : index, 7, 42, %arg42 : i32]":
+///       1. `result` is filled with the i64 ArrayAttr `[kDynamic, 7, 42,
+///       kDynamic]`
+///       2. `values` is filled with "[%arg0, %arg1]".
+///       3. `scalables` is filled with `[false, true, false]`.
+///
+///   * After parsing `[2, [4], 8]`:
+///       1. `result` is filled with the i64 ArrayAttr `[2, 4, 8]`
+///       2. `values` is empty.
+///       3. `scalables` is filled with `[false, true, false]`.
 ParseResult parseDynamicIndexList(
     OpAsmParser &parser,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &values,
-    DenseI64ArrayAttr &integers, DenseBoolArrayAttr &scalableVals,
+    DenseI64ArrayAttr &integers, DenseBoolArrayAttr &scalables,
     SmallVectorImpl<Type> *valueTypes = nullptr,
     AsmParser::Delimiter delimiter = AsmParser::Delimiter::Square);
 inline ParseResult parseDynamicIndexList(
@@ -149,28 +168,27 @@ inline ParseResult parseDynamicIndexList(
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &values,
     DenseI64ArrayAttr &integers, SmallVectorImpl<Type> *valueTypes = nullptr,
     AsmParser::Delimiter delimiter = AsmParser::Delimiter::Square) {
-  DenseBoolArrayAttr scalableVals = {};
-  return parseDynamicIndexList(parser, values, integers, scalableVals,
-                               valueTypes, delimiter);
+  DenseBoolArrayAttr scalables;
+  return parseDynamicIndexList(parser, values, integers, scalables, valueTypes,
+                               delimiter);
 }
 inline ParseResult parseDynamicIndexList(
     OpAsmParser &parser,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &values,
     DenseI64ArrayAttr &integers, SmallVectorImpl<Type> &valueTypes,
     AsmParser::Delimiter delimiter = AsmParser::Delimiter::Square) {
-  DenseBoolArrayAttr scalableVals = {};
-  return parseDynamicIndexList(parser, values, integers, scalableVals,
-                               &valueTypes, delimiter);
+  DenseBoolArrayAttr scalables;
+  return parseDynamicIndexList(parser, values, integers, scalables, &valueTypes,
+                               delimiter);
 }
 inline ParseResult parseDynamicIndexList(
     OpAsmParser &parser,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &values,
     DenseI64ArrayAttr &integers, SmallVectorImpl<Type> &valueTypes,
-    DenseBoolArrayAttr &scalableVals,
+    DenseBoolArrayAttr &scalables,
     AsmParser::Delimiter delimiter = AsmParser::Delimiter::Square) {
-
-  return parseDynamicIndexList(parser, values, integers, scalableVals,
-                               &valueTypes, delimiter);
+  return parseDynamicIndexList(parser, values, integers, scalables, &valueTypes,
+                               delimiter);
 }
 
 /// Verify that a the `values` has as many elements as the number of entries in

[matthias-springer]

Maybe worth noting that if valueTypes is not provided, types are not parsed at all. I.e., %arg0 : index is a syntax error.

[matthias-springer]

I would add a note here, that valueTypes could be deduced from values, but we have to make sure that the API for the parse/print functions is symmetric.

[banach-space]

Thanks!

[banach-space]

Shall we rename scalables as scalableFlags? The latter is more descriptive and, in general, makes more sense to me. Also, apologies for introducing scalables 😅

[banach-space]

Would it make sense to add a note that "scalable index" makes only sense in the context of "scalable vectors"? It's very clear to me, but might be confusing to folks who don't work with VectorType.