[PATCH] D146724: [mlir][MemRef] Add patterns to extract address computations

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache requested changes to this revision.
nicolasvasilache added a comment.
This revision now requires changes to proceed.

sorry clicked too soon, added some comments



================
Comment at: mlir/include/mlir/Dialect/MemRef/Transforms/Transforms.h:35
+/// ```
+void populateExtractAddressComputationsPatterns(RewritePatternSet &patterns);
+
----------------
also move the other populates here ?


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/ExtractAddressComputations.cpp:118
+// Matches getFailureOrSrcMemRef specs for TransferReadOp.
+// \see LoadStoreLikeOpRewriter.
+template <typename TransferLikeOp>
----------------
For my own education, could you please point me to explanations about `\see` and other similar things I have seen you use over time ?
I'd happily follow suit and try to improve my own comments.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/ExtractAddressComputations.cpp:223
+template <typename LoadStoreLikeOp,
+          FailureOr<Value> (*getFailureOrSrcMemRef)(LoadStoreLikeOp),
+          LoadStoreLikeOp (*rebuildOpFromAddressAndIndices)(
----------------
llvm::function_ref instad of C style funptrs ?


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/ExtractAddressComputations.cpp:246
+    if (loadStoreRank == 0)
+      return failure();
+
----------------
`return rewriter.notifyMatchFailure(op, "...")` everywhere plz, this greatly simplifies understanding when debugging


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/ExtractAddressComputations.cpp:269
+                                           /*offsets=*/indices,
+                                           /*sizes=*/sizes, /*strides=*/ones);
+    // Rewrite the load/store with the subview as the base pointer.
----------------
Add a good comment that strides are handled properly by composition of subviews here please.
This is not true for the reverse transformation and the precondition for vector.transfer checks and fails to match if strides are not all one: the forward and inverse problems are not symmetrical unfortunately.



================
Comment at: mlir/test/Dialect/MemRef/extract-address-computations.mlir:287
+  %vcf0 = arith.constant dense<0.000000e+00> : vector<4x2xf16>
+  %res = vector.transfer_write %vcf0, %base[%offset0, %offset1, %offset2] { permutation_map = affine_map<(d0,d1,d2) -> (d2,d0)> } : vector<4x2xf16>, tensor<?x?x?xf16>
+  return %res : tensor<?x?x?xf16>
----------------
some examples with strides please that mirror my previous comment


================
Comment at: mlir/test/lib/Dialect/MemRef/TestExtractAddressComputations.cpp:29
+
+struct TestExtractAddressComputations
+    : public PassWrapper<TestExtractAddressComputations,
----------------
can we just wrap this in a transform op and avoid creating unused code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146724