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

[Sean Silva via Phabricator via llvm-commits]

silvas marked 5 inline comments as done.
silvas added inline comments.


================
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
----------------
jpienaar wrote:
> 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()).
Good catch. We were using I64ElementsAttr for representing shapes when constant folding, but we should have been using IndexElementsAttr (which doesn't exist yet). I'll add IndexElementsAttr to core and fix this as part of the upgrade the shape dialect to using that.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:298
+  Builder builder(getContext());
+  return builder.getIntegerAttr(
+      builder.getIndexType(),
----------------
tpopp wrote:
> There is a `getIndexAtrr(int64_t)` method that you could use here.
it is just elements.getValue({dimToGet}) after the IndexElementsAttr fix (patches will be uploaded soon).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80394