[PATCH] D78754: [shape] Basic constant folding.

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:157
 
-def Shape_ConstantOp : Shape_Op<"constant", []> {
-  let summary = "Creates a shape constant";
+def Shape_ConstSizeOp : Shape_Op<"const_size",
+    [ConstantLike,
----------------
Let's keep these sorted


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:166
 
-  // TODO: Move this to main so that all shape ops implement these.
   let printer = [{ return ::print(p, *this); }];
----------------
This hasn't been resolved and so should still be left todo


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:88
 //===----------------------------------------------------------------------===//
-// Constant*Op
+// ShapeOfOp
 //===----------------------------------------------------------------------===//
----------------
Let's keep the ops sorted alphabetical


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:92
+OpFoldResult ShapeOfOp::fold(ArrayRef<Attribute>) {
+  auto type = getOperand().getType().cast<ShapedType>();
+  if (!type.hasStaticShape())
----------------
This op doesn't operate only on ShapedType so use dyn_cast instead.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:111
+    extents.push_back(
+        IntegerAttr::get(IntegerType::get(64, op.getContext()), extent));
+  }
----------------
mehdi_amini wrote:
> Having a builder and using `getI64IntegerAttr(int64_t value);` seems like it'd help here
Seems wasteful to construct an arrayattr and unique it in context just to print, an interleaveWithComma call should suffice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78754