[PATCH] D72432: [mlir] Add shaped container component type interface

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Just looked at the doc for now, will look at the rest later.



================
Comment at: mlir/docs/ShapeInference.md:4
+Shape inference as discussed here is considered a specific instance of type
+inference for ShapedType. Type constraints are along (at least) three axis: 1)
+elemental type, 2) rank (including static or dynamic), 3) dimensions. While some
----------------
Is there a place that we can link to for ShapedType?


================
Comment at: mlir/docs/ShapeInference.md:6
+elemental type, 2) rank (including static or dynamic), 3) dimensions. While some
+ops have no compile time fixed shape (e.g., output shape is dictated by data) we
+could still have some knowledge of constraints/bounds in the system for that op
----------------
nit: expand ops to operations


================
Comment at: mlir/docs/ShapeInference.md:7
+ops have no compile time fixed shape (e.g., output shape is dictated by data) we
+could still have some knowledge of constraints/bounds in the system for that op
+(e.g., the output of a `tf.where` is at most the size of the input data). And so
----------------
op -> operation


================
Comment at: mlir/docs/ShapeInference.md:8
+could still have some knowledge of constraints/bounds in the system for that op
+(e.g., the output of a `tf.where` is at most the size of the input data). And so
+there are additional valuable constraints that could be captured even without
----------------
nit: Can you reword to avoid starting with 'And'.


================
Comment at: mlir/docs/ShapeInference.md:13
+Type inference is currently modelled executionally for op creation using the
+`InferTypeOpInterface`, while `InferShapedTypeOpInterface` is used to implement
+the shape inference operations. The return type can often be deduced from the
----------------
Is there a link for `InferShapedTypeOpInterface`? Maybe we should start generating/improve the docs for interfaces.


================
Comment at: mlir/docs/ShapeInference.md:16
+deduced return shape and elemental type (queryable from
+`InferShapedTypeOpInterface`), so that a tensor type inference can be
+implemented using shape inference.
----------------
Can you reword? This sentence sounds a bit weird. The second part doesn't seem to flow from the first.


================
Comment at: mlir/docs/ShapeInference.md:26
+
+*   Constraint on the operands of an operation directly. For example
+    constraining the input type to be tensor/vector elements or that the
----------------
nit: Constraints


================
Comment at: mlir/docs/ShapeInference.md:28
+    constraining the input type to be tensor/vector elements or that the
+    elemental type be of a specific type (e.g., output of sign is of elemental
+    type `i1`) or class (e.g., float like).
----------------
Is `sign` a know thing we can link to?


================
Comment at: mlir/docs/ShapeInference.md:30
+    type `i1`) or class (e.g., float like).
+*   Constraints across operands and results of an operation. For example,
+    enabling specifying equality constraints on type/constituents of a type
----------------
nit: Could we move the 'For example...' parts to a sub-bullet?


================
Comment at: mlir/docs/ShapeInference.md:35
+
+The C++ shape functions are an intermediate step until the shape dialect is more
+full-fledged, at which point the C++ functions should become the exceptional
----------------
Should this be a `NOTE:`?


================
Comment at: mlir/docs/ShapeInference.md:54
+
+But shape functions are determined by attributes and could be arbitrarily
+complicated with a wide-range of specification possibilities. Equality
----------------
`But` seems like a weird start to a new section. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72432