[PATCH] D72168: [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/Dialect/Linalg/IR/LinalgOps.td:109
+    static StringRef getReassociationAttrName() { return "reassociation"; }
+    MemRefType getViewType() { return view()->getType().cast<MemRefType>(); }
+  }];
----------------
Do not add more uses of -> or *, this is just giving me more work.


================
Comment at: mlir/include/mlir/IR/AffineExpr.h:248
+  if (!*this)
+    return U(nullptr);
+  if (isa<U>())
----------------
return (!*this || !isa<U>()) ? U(nullptr) : U(expr);


================
Comment at: mlir/include/mlir/IR/Attributes.h:1444
+/// corresponding vector.
+SmallVector<AffineMap, 4> getAffineMaps(ArrayAttr attrs);
+
----------------
Seems like it would be better to have something more general on ArrayAttr that can unwrap internal attributes. Preferably by returning a range instead of creating a vector.


================
Comment at: mlir/include/mlir/IR/Builders.h:381
+/// corresponding vector.
+SmallVector<AffineMap, 4> getAffineMaps(ArrayAttr attrs);
+
----------------
I don't see a need for this.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:15
 #include "mlir/Support/STLExtras.h"
+#include "third_party/llvm/llvm-project/llvm/include/llvm/ADT/SmallVector.h"
+#include "third_party/llvm/llvm-project/mlir/include/mlir/Support/LogicalResult.h"
----------------
These paths make no sense.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:577
+  if (!affineMaps.empty()) {
+    auto m = affineMaps.front();
+    unsigned numDims = m.getNumDims();
----------------
This needs a comment, specifically I don't see why we need to do all of this simplification here.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:582
+    offset = simplifyAffineExpr(offset, numDims, numSymbols);
+    for (auto &stride : strides)
+      stride = simplifyAffineExpr(stride, numDims, numSymbols);
----------------
Why by-reference?


================
Comment at: mlir/lib/IR/StandardTypes.cpp:763
+/// layout.
+bool mlir::isContiguous(MemRefType t) {
+  return canonicalizeStridedLayout(t).getAffineMaps().empty();
----------------
These names seem way too general for what they are used for. Why are these free functions and not methods of MemRefType?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72168