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

[Sean Silva via Phabricator via llvm-commits]

silvas requested changes to this revision.
silvas added a comment.
This revision now requires changes to proceed.

For future reference, this patch should probably be multiple separate patches:

1. moving ConcatOp
2. adding size_to_index and size_from_index
3. adding num_elements
4. Tidying up docs of Shape_JoinOp and Shape_ReduceOp and Shape_ConstSizeOp
5. the change to StandardTypes.h

That helps keep each review focused which results in much better review feedback. It also helps people retroactively working through the code history to understand the development process, or folks trying.

I would appreciate if you do it for this patch already; I think there's more feedback to be gotten that is being masked by the mix of stuff happening in this patch. At the very least the commit description needs to be updated to incorporate the fact that it is doing all this stuff.



================
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
----------------
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)


================
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
----------------
You don't seem to use these captured FileCheck variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80004