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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 3 inline comments as done.
nicolasvasilache added inline comments.


================
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;
----------------
ftynse wrote:
> If this dim hasn't been seen yet, can strides[dim] be something other than zero? If not, just assign 1 to it.
Now that I accumulate AffineExpr symbolically, the assertion on seen is incorrect, thanks!
In fact I can remove seen everywhere and just check against 0 in the end.


================
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:
> 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.



================
Comment at: mlir/lib/IR/StandardTypes.cpp:542
       return success();
-    }
     stridedExpr = makeCanonicalStridedLayoutExpr(t.getShape(), t.getContext());
+  } else {
----------------
ftynse wrote:
> (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.
This was already almost the case.
I pushed simplification a bit further and got to what you suggested.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72098