[PATCH] D80004: [MLIR] Add `from_index`, `to_index`, and `num_elements` to the shape dialect

[Frederik Gossen via Phabricator via llvm-commits]

frgossen marked 8 inline comments as done.
frgossen added a comment.

As suggested by @silvas , this revision is now split up into multiple ones:

- [MLIR] Cosmetic change: https://reviews.llvm.org/D80275
- [MLIR] Move `ConcatOp` to its lexicographic position: https://reviews.llvm.org/D80277
- [MLIR] Tidy up documentation for `Shape_JoinOp`, `Shape_ReduceOp`, and `Shape_ConstSizeOp`: https://reviews.llvm.org/D80278
- [MLIR] Add `index_to_size` and `size_to_index` to the shape dialect: https://reviews.llvm.org/D80280?vs=on&id=265167
- [MLIR] Add `num_elements` to the shape dialect: https://reviews.llvm.org/D80281
- [MLIR] Fix operand type in `from_extent_tensor` in the shape dialect: https://reviews.llvm.org/D80283



================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:156
+
+def Shape_SizeToIndexOp : Shape_Op<"size_to_index", [
+    NoSideEffect,
----------------
jpienaar wrote:
> Could we keep these sorted?
See https://reviews.llvm.org/D80280


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:312
+  // Fold only when argument constant.
+  Attribute arg = operands[0];
+  if (!arg)
----------------
jpienaar wrote:
> It would seem for these,
> 
> if (Attribute arg = operands[0]) return arg;
> return {};
> 
> is simpler.
See https://reviews.llvm.org/D80280


================
Comment at: mlir/test/Dialect/Shape/canonicalize.mlir:119
+  %cs0 = shape.index_to_size %ci0
+  // CHECK: constant 123 : index
+  %ci1 = shape.size_to_index %cs0
----------------
silvas wrote:
> For this specific test, you might want to specifically check that the value is returned directly (otherwise the test also passes if no transformation is made)
See https://reviews.llvm.org/D80280


================
Comment at: mlir/test/Dialect/Shape/canonicalize.mlir:128
+func @nonfoldable_size_to_index(%cs : !shape.size) -> index {
+  // CHECK: %[[CI:.*]] = shape.size_to_index %[[CS:.*]]
+  %ci = shape.size_to_index %cs
----------------
silvas wrote:
> You don't seem to use these captured FileCheck variables.
See https://reviews.llvm.org/D80280


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80004