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

[Jacques Pienaar via Phabricator via llvm-commits]

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

Thanks!



================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:76
     * `[*]` if it is an unranked shape
     * `[?, 2]` if a rank 2 tensor with one unknown dimension
     * `[3, 4]` is a rank 2 static tensor
----------------
I'd like to see tests showing each of these being printed (well OK with invalid one not being shown), but it wouldn't be possible without custom parser, so OK to start with the current version.


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:98
+  p << "[";
+  interleaveComma(op.shape().getValues<int64_t>(), p,
+                  [&](int64_t i) { p << i; });
----------------
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


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:112
+  SmallVector<NamedAttribute, 6> dummy;
+  if (parser.parseAttribute(extentsRaw, "dummy", dummy))
     return failure();
----------------
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


================
Comment at: mlir/lib/Dialect/Shape/IR/Shape.cpp:119
+  for (Attribute extent : extentsArray) {
+    IntegerAttr attr = extent.dyn_cast<IntegerAttr>();
+    if (!attr)
----------------
This seems equally long to

  if (auto attr = extent.dyn_cast<IntegerAttr>())
    ints.push_back(attr.getInt());
  else
    return failure();

Don't know which one reads easier [this one feels like variable usage scope is more local and makes the two cases clearer but thats just a feelign and not a trong one]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78754