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

[Frederik Gossen via Phabricator via llvm-commits]

frgossen marked 3 inline comments as done.
frgossen added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:109
 
+def Shape_FromIndexOp : Shape_Op<"from_index", []> {
+  let summary = "Creates a shape size from a standard index";
----------------
herhut wrote:
> pifon2a wrote:
> > silvas wrote:
> > > These should be marked NoSideEffect and have folders and InferTypeOpInterface. See https://reviews.llvm.org/D79833 for an example of another op.
> > I am not sure `from_index` is enough. In the IR i first read it as if it creates 1D shape with the provided number number of elements. Maybe call this `from_index_rank`?
> > 
> > Is there some special reason why we have `shape.size` instead of `shape.rank`?
> Maybe `index_to_size` and `size_to_index` would better convey what this does. We could also have a single operation `size_cast` in the spirit of the `index_cast` operation in standard that allows both conversions. That would be more uniform and, if we get to a state where we can have a type trait that would allow to use 'index_cast', it would be easier to rewrite.
@silvas `NoSideEffect` makes perfect sense and I will also add the folders where possible. 
Can you explain what the `InferTypeOpInterface` is good for? 
I can copy and adapt the code, of course, but I would like to understand its benefit. 
Is the return type of, e.g., `Shape_FromIndexOp` not already known from the declaration `outs Index:$result`? 

@pifon2a As far as I understand it, `shape.size` is used to describe also but not only the rank of a tensor. 
The `shape.reduce` example, e.g., already uses it to represent the number of elements. 
Until now I thought of `shape.size` as `shape`'s equivalent to `size_t`. 
The name `from/to_index` was meant to be analogous to `from/to_extent_tensor` and `index` was supposed to refer to `std`'s `index`.
Would `from/to_std_index` be a better name?  




================
Comment at: mlir/lib/IR/Builders.cpp:57
 
+IntegerType Builder::getI32Type() { return IntegerType::get(32, context); }
+
----------------
silvas wrote:
> can you split this cleanup of the core infra into a different patch?
It's now in https://reviews.llvm.org/D80111
I will remove the change here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80004