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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/IR/Attributes.h:1444
+/// corresponding vector.
+SmallVector<AffineMap, 4> getAffineMaps(ArrayAttr attrs);
+
----------------
rriddle wrote:
> 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.
Sorry I have to punt on this one, I looked at the iterator_facade stuff but this exceeds my immediate C++ skills.


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


================
Comment at: mlir/lib/IR/StandardTypes.cpp:582
+    offset = simplifyAffineExpr(offset, numDims, numSymbols);
+    for (auto &stride : strides)
+      stride = simplifyAffineExpr(stride, numDims, numSymbols);
----------------
rriddle wrote:
> Why by-reference?
This was a bad rebase, should be good now.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72168