[PATCH] D80280: [MLIR] Add `index_to_size` and `size_to_index` to the shape dialect

[Frederik Gossen via Phabricator via llvm-commits]

frgossen added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:294
+    This operation and its inverse, `index_to_size`, facilitate index conversion
+    between the standard and the shape dialect.
+    Expects a `shape.size` and returns an `index` value.
----------------
frgossen wrote:
> herhut wrote:
> > jpienaar wrote:
> > > size represents a non-negative integer with support for being unknown and invalid, what is the conversion/behavior when the operand is invalid?
> > If the size is invalid, then I'd assume that all code that depends on it has undefined behavior. In particular, `size_to_index` should have an undefined result. For the inverse, we could consider checking that the `index` argument is indeed positive or clamp it at [0,inf). That has the drawback that we cannot compile the conversion from index to size into a no-op.
> > 
> > I would prefer not to expose the internal representation of invalid (-1 I believe) at runtime. However, if we go for undefined behavior, returning -1 would be valid.
> As there exists no lowering at the moment, it seems that all of these operations' semantics are embedded in the constant folding mechanism. 
> Would it be okay to say the behaviour is undefined in this case? 
> 
We could also say that the conversion from `index_to_size` is undefined if the argument is negative. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80280