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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:163
   let description = [{
+    Creates a constant of !shape.size type.
     An operation that builds a size or shape from integer or array attribute.
----------------
I don't think we should repeat the summary in the description?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:164
+    Creates a constant of !shape.size type.
     An operation that builds a size or shape from integer or array attribute.
     It allows for creating dynamically valued shapes by using `?` for unknown
----------------
This needs an update.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:169
     ```mlir
     %x = shape.constant 10 : !shape.size
     ```
----------------
This example needs an update.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:185
+  let description = [{
+    Creates a constant of !shape.shape type.
+  }];
----------------
It'd be nice to provide the long description like the other above (these will show up on the website)


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:39
+                                        value.cast<DenseIntElementsAttr>());
+  }
+  return nullptr;
----------------
Should handle SizeType here as well?


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:111
+    extents.push_back(
+        IntegerAttr::get(IntegerType::get(64, op.getContext()), extent));
+  }
----------------
Having a builder and using `getI64IntegerAttr(int64_t value);` seems like it'd help here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78754