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

[Sean Silva via Phabricator via llvm-commits]

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


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:98
+  p << "[";
+  interleaveComma(op.shape().getValues<int64_t>(), p,
+                  [&](int64_t i) { p << i; });
----------------
jpienaar wrote:
> This should use ? for unknown and regular integer for everything else. Adding support for this would mean the IR can't roundtrip, but please add a TODO for follow up
See other comment.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:112
+  SmallVector<NamedAttribute, 6> dummy;
+  if (parser.parseAttribute(extentsRaw, "dummy", dummy))
     return failure();
----------------
jpienaar wrote:
> Unknown's should be parsed as ? in the descriptions so we would need some additional logic here. E.g., -1 shouldn't be used in the textual form. So a more custom parsing would be required, could you add a TODO here too
I don't understand the use case for shape constants of partially unknown shape. Maybe we can discuss on discourse?


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:162
+  Builder builder(getContext());
+  return builder.getI64TensorAttr(type.getShape());
+}
----------------
jpienaar wrote:
> What happens here if we have a error shape fed into ShapeOfOp?
We check that the tensor operand has static shape. So it can't be an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78754