[PATCH] D73572: [mlir] Expand shape functions in ShapeInference doc

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/docs/ShapeInference.md:57
+This section details the shape type inference dialect (`shape`). The initial
+focus will be on shape functions that describe shape functions could be used in
+runtime and compiler (for constructions of ops/refinement of shapes, reification
----------------
This will likely need to be reworded in the near future.


================
Comment at: mlir/docs/ShapeInference.md:63
+This will focus on the shape functions (e.g., determine the rank and dimensions
+of the output shape). As shown in the shaped container type, shape will be one
+of 3 components, the others being elemental type and attribute (which is
----------------
What is the "shaped contained type"? Link?


================
Comment at: mlir/docs/ShapeInference.md:64
+of the output shape). As shown in the shaped container type, shape will be one
+of 3 components, the others being elemental type and attribute (which is
+currently left open with the intention of supporting extensions such as layouts
----------------
nit: being the element type


================
Comment at: mlir/docs/ShapeInference.md:72
+    with the others);
+*   It allows reusing the constraints between, say, Tensor and Memref
+    representation of an operation;
----------------
Link to each of these types.


================
Comment at: mlir/docs/ShapeInference.md:77
+functions, with some considering shape and elemental type different and some as
+part of shape. But `shape function` is IMHO descriptive and metadata can span
+too large a range of potential uses/values.
----------------
Can we remove IMHO and reword this to avoid starting with a 'But'?


================
Comment at: mlir/docs/ShapeInference.md:82
+
+The requirements for the shape inference functions are shaped by the
+requirements of shape inference, but we believe the requirements below still
----------------
shaped -> guided (or is the pun intended?)


================
Comment at: mlir/docs/ShapeInference.md:91
+    have shapes that are not known statically (for example, `tensor<16x?xf32>`
+    or `tensor<*xf32>*`);
+*   **Shape error detection** Many operations will have constraints on their
----------------
Is the last * intended?


================
Comment at: mlir/docs/ShapeInference.md:115
+        description should be consumable by either.
+    *   Shape function description should not be constrained by either runtime
+        or compiler's type system to handle types only used for analysis. That
----------------
nit: descriptions


================
Comment at: mlir/docs/ShapeInference.md:137
+
+        This requirement is on the wishlist.
+
----------------
Wrap in ()?


================
Comment at: mlir/docs/ShapeInference.md:145
+        only (e.g., same operands as the corresponding operation could be
+        constructed & invoked with).
+    *   Shape information that need higher-level/graph information should use
----------------
& -> and


================
Comment at: mlir/docs/ShapeInference.md:147
+    *   Shape information that need higher-level/graph information should use
+        richer types (e.g., `TensorList<F32>`);
+    *   The function should be invocable before/while constructing an op (e.g.,
----------------
TensorList isn't a well known thing, e.g. I don't know what this point represents.


================
Comment at: mlir/docs/ShapeInference.md:165
+        attributes such as padding etc.) that will be discovered and could cover
+        a large percentage of the use cases. And among these there will be some
+        which carry extra semantic info that could be used for symbolic
----------------
nit: It's a bit weird to start with an And.


================
Comment at: mlir/docs/ShapeInference.md:195
+
+1.  The shape dialect is an IR representations and not a programming language;
+    *   While the functions should be readable, it doesn't carry the
----------------
representation


================
Comment at: mlir/docs/ShapeInference.md:205
+        static information is used for shape output, unranked for everything
+        else) to very advance (e.g., expression trees of symbolic constants) can
+        be evaluated independently of this proposal and with concrete benefit
----------------
advanced


================
Comment at: mlir/docs/ShapeInference.md:212
+        deciding whether or which error to emit: there have been proposals in
+        the literature that the iteration order for shape inference affect the
+        quality of the error message produced, and the shape functions do not
----------------
link to such literature?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73572/new/

https://reviews.llvm.org/D73572