[PATCH] D79062: [mlir][Vector] Provide progressive lowering of masked n-D vector transfers

[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/EDSC/Builders.h:116
   /// let the escape.
   /// Step back "prev" times from the end of the block to set up the insertion
   /// point, which is useful for non-empty blocks.
----------------
This comment needs an update


================
Comment at: mlir/include/mlir/EDSC/Builders.h:124
+    if (!block->empty())
+      if (auto *termOp = block->getTerminator())
+        ScopedContext::getBuilder().setInsertionPoint(termOp);
----------------
This will crash if a block has an operation, which is not known to be a terminator (which may happen in rewrite patterns).  I would prefer a more cautious way: get the last operation, check if it is a known terminator, and if so, update the insertion point.


================
Comment at: mlir/include/mlir/EDSC/Builders.h:200
   /// Enters the mlir::Block* previously captured by `bh` and sets the insertion
-  /// point to its end.
+  /// point to its end. If the block already contains a terminator, set thes
+  /// insertion point before the terminator.
----------------
Typo: "thes"


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:62
+///          %ivs_minor):
+///         memref<leading_dims) x (major_dims) x (minor_dims) x type>,
+///         vector<(minor_dims) x type>;
----------------
Here and below + in the commit description, the type spelling `memref<leading_dims) x (major_dims) x (minor_dims) x type>` looks broken to me.


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:94
+    memRefMinorVectorType =
+        MemRefType::get(majorVectorType.getShape(), minorVectorType);
+  }
----------------
Is this memref always in the default address space? What if the memref we are transferring to/from is in the non-default address space?


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:98
+  template <typename Lambda>
+  void emitLoops(Lambda loopBody);
+
----------------
plz document here and below


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:101
+  /// Operate within the body of `emitLoops` to:
+  /// 1. Compute whether the first `majorIvs.rank()` dimensions `majorIvs +
+  /// majorOffsets` are all within `memrefBounds`.
----------------
Nit: this "1." without any continuation in the list looks fishy


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:104
+  template <typename LambdaThen, typename LambdaElse>
+  void emitInBounds(ArrayRef<Value> majorIvs, ArrayRef<Value> majorOffsets,
+                    MemRefBoundsCapture &memrefBounds,
----------------
ValueRange?


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:153
+    LambdaElse elseBlockBuilder) {
+  Value inBounds = std_constant_int(1, 1);
+  SmallVector<Value, 4> majorIvsPlusOffsets;
----------------
Nit: please add `/*name=*/` annotations to differentiate which `1` corresponds to what


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:184
+      auto map = AffineMap::getMinorIdentityMap(
+          xferOp.getMemRefType().getRank(), 1, xferOp.getContext());
+      // Lower to 1-D vector_transfer_read and let recursion handle it.
----------------
Maybe use `minorRank` instead of hardcoded `1` here?


================
Comment at: mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:579
 
-void mlir::populateVectorToAffineLoopsConversionPatterns(
-    MLIRContext *context, OwningRewritePatternList &patterns) {
+void mlir::populateVectorToLoopsLoweringPatterns(
+    OwningRewritePatternList &patterns, MLIRContext *context) {
----------------
Why rename `Conversion` to `Lowering`? We decided to use the former instead of the latter....


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1486
+                      getElementTypeOrSelf(getElementTypeOrSelf(memRefType)));
+  result.addTypes(MemRefType::get({}, vectorType));
 }
----------------
Should we also carry over the address space of the original memref type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79062