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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/include/mlir/Dialect/MemRef/Transforms/Transforms.h:35
+/// ```
+void populateExtractAddressComputationsPatterns(RewritePatternSet &patterns);
+
----------------
nicolasvasilache wrote:
> also move the other populates here ?
I thought we would do that in an NFC commit.
Which I'll do after that one land.


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/ExtractAddressComputations.cpp:118
+// Matches getFailureOrSrcMemRef specs for TransferReadOp.
+// \see LoadStoreLikeOpRewriter.
+template <typename TransferLikeOp>
----------------
nicolasvasilache wrote:
> 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.
Here is the complete doc for the doxygen "keywords":
https://www.doxygen.nl/manual/commands.html

The ones I use that may not be that common are:
- \pre, \post: Document a pre-post condition
- \see: Create a link to another item (class, function, etc.) also documented in doxygen

But maybe what I think is common or not is biased :P (\note, \code, \p, etc.).


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/ExtractAddressComputations.cpp:223
+template <typename LoadStoreLikeOp,
+          FailureOr<Value> (*getFailureOrSrcMemRef)(LoadStoreLikeOp),
+          LoadStoreLikeOp (*rebuildOpFromAddressAndIndices)(
----------------
nicolasvasilache wrote:
> llvm::function_ref instad of C style funptrs ?
Good point, I always forgot about these!
(I start with std::function, then remember that templates don't like them and resort to plain C. I need to update my mental model :)).


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


================
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.
----------------
nicolasvasilache wrote:
> 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>
----------------
nicolasvasilache wrote:
> 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,
----------------
nicolasvasilache wrote:
> can we just wrap this in a transform op and avoid creating unused code?
Sure thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146724