[PATCH] D72040: [mlir][Linalg] Add a linalg.reshape op

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/IR/Builders.h:380
+/// Helper functions assert Attribute of the proper type in attr and returns the
+/// corresponding vector.
+SmallVector<AffineMap, 4> getAffineMaps(ArrayAttr attrs);
----------------
I don't see why this should be here. This seems unrelated to Builders.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:351
+  assert(sizes.size() == strides.size() && "mismatched ranks");
+  assert(sizes.size() == strides.size() && "mismatched ranks");
+  // off by 1 indexing to avoid out of bounds
----------------
Duplicated assert.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:354
+  //                       V
+  for (auto idx = dim; idx + 1 < dim + extent; ++idx) {
+    // All values involved must be static, symbolic dims would require more
----------------
Cache the end iterator.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:367
+
+static MemRefType computeReshapeResultType(MemRefType type,
+                                           ArrayRef<AffineMap> reassociation) {
----------------
This should have a comment.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:383
+  assert(isReassociationValid(reassociation) && "invalid reassociation");
+  for (auto m : reassociation) {
+    unsigned dim = m.getNumResults();
----------------
nit: use AffineMap instead of auto


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:406
+  auto maps = getAffineMaps(reassociation);
+  auto memRefType = view->getType().cast<MemRefType>();
+  auto resultType = computeReshapeResultType(memRefType, maps);
----------------
Do not use -> or * on Value unless you really need to. They are going away and it just adds extra work for me. I see many other places in this revision that also use them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72040