[PATCH] D79662: [mlir] Revisit std.subview handling of static information.

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added a comment.
This revision now requires changes to proceed.

Mostly looks OK to me and much better to canonicalize. Thanks for doing this. Some minor comments.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2451
+    *) Strides: memref-rank number of dynamic strides or static integer
+              attributes multiplicatively to the base memref strides in each
+              dimension.
----------------
Nit: please align as above.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:2561
+      "ValueRange offsets, ValueRange sizes, ValueRange strides, "
+      "ArrayRef<int64_t> staticOffsets = ArrayRef<int64_t>(), "
+      "ArrayRef<int64_t> staticSizes = ArrayRef<int64_t>(), "
----------------
AFAIK the verifier code below, the staticOffsets, staticSizes and staticStrides need to be specified always. So having a default value here is misleading. Can we move these operands before the offsets, sizes and strides (I know that makes it a breaking change)


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2341
+  // Fill with the appropriate dynamic marker.
+  if (staticOffsetsVector.empty())
+    staticOffsetsVector.assign(rank, ShapedType::kDynamicStrideOrOffset);
----------------
I find this a bit misleading. If left empty in the build, then this implicitly means that all the values are -1. I would rather be explicit that the subview needs an ArrayAttr for staticOffsets, staticSizes, staticStrides that is of size rank of the source. Based on what are -1, will dictate the sizes of offsets, sizes and strides that are specified. This would make the dynamic values optional and needed only if one of the static* has a -1 entry. Note this is more a suggestion.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2390
+    res.push_back(attr.getInt());
+  return res;
 }
----------------
isnt this just

llvm::to_vector<4>(llvm::map_range(attr, [](Attribute a) -> int64_t { return a.cast<IntegerAttr>().getInt();}))


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:2508-2509
+    SmallVector<int64_t, 8> newStaticOffsets =
+        extractFromI64ArrayAttr(subViewOp.static_offsets());
+    assert(newStaticOffsets.size() == subViewOp.getRank());
+    canonicalizeSubViewPart(newOffsets, newStaticOffsets,
----------------
These two lines can be moved into the canonicalizeSubViewPart method since they are repeated below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79662