[PATCH] D73944: [mlir][wip] Start Shape dialect

[Stephan Herhut via Phabricator via llvm-commits]

herhut added a comment.

Great to see progress on this! Some comments.



================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:46
+  let typeDescription = [{
+    `shape.dim` represents a dimension type which could be either a non-negative
+    integer, unknown or error.
----------------
Nit: It is not used for dimensions, only, but generally as a positive integer value with support for being unknown and invalid. In that sense it is more like a `size_t` than a dim. Maybe `shape.size`? 


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:55
+
+def Shape_Type : DialectType<ShapeDialect,
+    CPred<"$_self.isa<ShapeType>()">, "shape"> {
----------------
I find the name type somewhat misleading. I think `Shape_Shape` would describe better what it is (a shape). But I get the desire to name it type. Should they all be `Shape_DimType` and `Shape_ShapeType`?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:100
+
+def Shape_DimOrType: AnyTypeOf<[Shape_Dim, Shape_Type], "dim or type">;
+
----------------
`DimOrShape`?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:110
+
+// TODO: consider just making a constant op instead.
+def Shape_ConstantDimOp : Shape_Op<"constant_dim", []> {
----------------
+1 to the constant op. Also, why expose a -1 here? Why not use

shape.constant unknown : !shape.size
shape.constant invalid : !shape.size
shape.constant <positive integer> : !shape.size

and then parse that internally into some special integer values? This would leave more flexibility to change the backing type to something other than int later.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:134
+
+    Note: it is not possible to create an unranked shape using the op.
+  }];
----------------
Is there a reason for this? Also, thinking of library calls that return ranked tensors of statically unknown rank, this function would not allow me to create a shape for them. Should we also have a version that takes just the rank?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:181
+
+def Shape_BroadcastableOp : Shape_Op<"broadcastable", []> {
+  let summary = "Returns the broadcastable output shape";
----------------
antiagainst wrote:
> I'd typically think an op means some action so the name is typically a verb but here we have `broadcastable` which sounds like a trait. What about just name it as `broadcast`?
I agree. Broadcast seems a better name, as it broadcasts two shapes into a resulting broadcasted shape.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:207
+// easier for folks.
+def Shape_JoinOp : Shape_Op<"join", []> {
+  let summary = "Returns the least general shape/dim of its operands";
----------------
antiagainst wrote:
> My 2c: join sounds better for me as it indicates some operation behind. any_of gives me the feeling that the result will be the same as one of the input, which is not always true. :)
`any_of` is less constrained in that it does not have to pick the join of the two values, whereas `join` has rigid semantics. On the other hand, the description below describes the join of the two values.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:221
+    shape.join([*], [1, 2]) -> [1, 2]
+    shape.join([], X) -> []
+    shape.join([1, U], [2, U, U]) -> []
----------------
I would expect 

join ([], []) -> []
join([], [*]) -> []
join([], [?,...,?]) -> [invalid]


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:222
+    shape.join([], X) -> []
+    shape.join([1, U], [2, U, U]) -> []
+    ```
----------------
join([1,?], [2, ?, ?]) -> [invalid] ?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:242
+    An operation that takes as input a shape, number of initial values and has a
+    region/function that is applied repeatedly for each every dimension of the
+    shape.
----------------
"for each every" is one too many.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:263
+        ^bb0(%index: i32, %dim: !si.dim, %lci: !shape.dim):
+          %acc = "shape.add"(%lci, %dim) :
+            (!shape.dim, !shape.dim) -> !shape.dim
----------------
Should this be `shape.mul` if it existed?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:271
+
+    If the shape is unknown, then the results of the op is also unknown.
+  }];
----------------
Does this mean the rank is unknown? Or also if parts of the shape are unknown? 

If known rank was allowed, one could still implement computing the rank by counting the elements of the vector (as a somewhat useless example). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73944