[PATCH] D72098: [mlir][Linalg] NFC - Reimplement getStridesAndOffset

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.
Herald added a subscriber: lucyrfox.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:472
+           "unexpected `seen` bit with single term");
+    strides[dim.getPosition()] = strides[dim.getPosition()] + 1;
+    seen[dim.getPosition()] = true;
----------------
If this dim hasn't been seen yet, can strides[dim] be something other than zero? If not, just assign 1 to it.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:476
   }
-  if (offset != MemRefType::getDynamicStrideOrOffset())
-    // Already seen case accumulates unless they are already saturated.
-    offset += val;
+  offset = offset + e;
+  seenOffset = true;
----------------
This looks like it could accept a hitherto non-canonical form of offset with multiple symbols -- N+M+K+42 -- and treat it as dynamic offset. Is this intentional? If so, please document. Otherwise, there may be a check on `seenOffset` missing somewhere.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:542
       return success();
-    }
     stridedExpr = makeCanonicalStridedLayoutExpr(t.getShape(), t.getContext());
+  } else {
----------------
(maybe later) I'd consider extracting a utility function from makeCanonicalStridedLayoutExpr that gives you strides as affine expressions, and reusing it here for immediate return instead of doing rounds of simplification on the expression you already know constructed from strides.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:545
+    auto m = affineMaps.front();
+    assert(m.getNumResults() == 1);
+    stridedExpr =
----------------
There's already a check in line 530


================
Comment at: mlir/lib/IR/StandardTypes.cpp:560
   }
+  if (!affineMaps.empty()) {
+    auto m = affineMaps.front();
----------------
There's a check that it has exactly one element in line 530


================
Comment at: mlir/lib/IR/StandardTypes.cpp:564
+    unsigned numSymbols = m.getNumSymbols();
+    assert(m.getNumResults() == 1);
+    offset = simplifyAffineExpr(offset, numDims, numSymbols);
----------------
There's a check above already


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72098