[PATCH] D80394: Add `shape.get_extent`.

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar accepted this revision.
jpienaar marked an inline comment as done.
jpienaar added a comment.
This revision is now accepted and ready to land.

Nice, thanks



================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:214
+        OpBuilder &builder, OperationState &result,
+        Value shape, int64_t dim
+      }],
----------------
This fits on one line

[I'm actually not sure why this one isn't generated by default, we have the generation for unwrapped types that I would have expected to trigger here ...]


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:290
+  uint64_t dimToGet = dim().getLimitedValue();
+  // TODO: Constant fold this to some kind of undefined behavior if we
+  // index out of range.
----------------
I'm not convinced of this unless we add this check to this op's verify method.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:294
+    return nullptr;
+  // This is a little inconvenient because getValue returns an IntegerAttr
+  // that is not of IndexType, but the result here needs to be of
----------------
What is the type? E.g., are we storing it wrong?

Is only shape.const_shape expected here? If not, then this conversion (and whether it is signed/unsigned/signless) seem to need to support more cases (based on the requirement of getInt()).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80394