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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
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;
----------------
nicolasvasilache wrote:
> ftynse wrote:
> > 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.
> Yes it is intentional: previously as I was doing my own saturation arithmetic I needed to special case.
> Now I just compute the symbolic expr and simplify in the end.
> 
Then I wouildn't call this diff "NFC", and rather add tests and user-visible documentation for these new cases. IMO, it deserves mentioning that (N+M+K + N*i0 + M*i1 - K*i1) gets stored as three values that are _not_ N,M,K.


================
Comment at: mlir/lib/IR/StandardTypes.cpp:464
+// i.e. single term). Accumulate the AffineExpr into the existing one.
+static LogicalResult extractStridesFromTerm(AffineExpr e,
+                                            MutableArrayRef<AffineExpr> strides,
----------------
This never fails, maybe just keep `void` as return type?


================
Comment at: mlir/lib/IR/StandardTypes.cpp:531
+  // Canonical case for empty/identity map.
+  if (!m || m.isIdentity()) {
+    // 0-D corner case, offset is already 0.
----------------
Nit: I remember writing code that drops identity maps from the layout, if it's still there, we shouldn't see them here. I'm fine with this code in any case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72098