[PATCH] D80728: [mlir][Linalg][Vector] Add forwarding patterns between linalg.copy and vector.transfer

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:454
+///    [optional] %view = std.view %alloc ...
+///    %subView = subview %A ...
+///    [optional] linalg.fill(%allocOrView, %cst) ...
----------------
Nit: %A is not defined here, did you mean {%alloc or %view}. Same in the commit description.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:458
+///    vector.transfer_read %allocOrView[...], %cst ...
+/// ```
+/// Where there is no interleaved use between linalg.copy transfer_read and no
----------------
Could you also mention what is this pattern rewritten _to_ ?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:106
 
+  StringRef dbgPref = "\n[" DEBUG_TYPE "]: ";
+  (void)dbgPref;
----------------
I wonder if we want to write some macro wrapper along the lines of

```
#ifdef DEBUG_TYPE
#define DBGS() \
  (dbgs() << '[' << DEBUG_TYPE << "] ")
#else
#define DBGS() \
  (dbgs())
#endif
```

and propose it for LLVM.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:140
+    LLVM_DEBUG(llvm::dbgs()
+               << dbgPref << "interleavedUses precondition failed, firstOp: "
+               << *firstOp << ", second op: " << *secondOp);
----------------
Please document the precondition somwhere


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:168
+
+  // Transfer into `view`.
+  Value viewOrAlloc = xferOp.memref();
----------------
I don't understand this comment


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp:261
+        return failure();
+      else
+        subViewOp = newSubViewOp;
----------------
aartbik wrote:
> as requested above by riddle
Maybe this can be factored out into a function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80728