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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache 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;
----------------
ftynse wrote:
> 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.
Ok then .. benhavior extend and tests added.


================
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.
----------------
ftynse wrote:
> 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.
ah cool, I'll update to assertions everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72098