[PATCH] D80100: [mlir][Vector] Add option to fully unroll for VectorTransfer to SCF lowering

[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/Vector/VectorOps.td:377
+    "ArrayRef<int64_t> position">,
+    // Convenience builder which assumes the values are constant indices.
+    OpBuilder<"OpBuilder &builder, OperationState &result, Value source,"
----------------
I was confused by this comment wrt the builder above. Consider "which assumes the values in `position` are defined by ConstantIndexOp`".


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:544
+    OpBuilder<
+    // Convenience builder which assumes all values are constant indices.
+    "OpBuilder &builder, OperationState &result, Value source, " #
----------------
The position of this comment is just weird


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:545
+    // Convenience builder which assumes all values are constant indices.
+    "OpBuilder &builder, OperationState &result, Value source, " #
+    "Value dest, ValueRange position">];
----------------
Nit: no need for `#`, string literals are auto-concatenated like in C


================
Comment at: mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp:146
+    for (unsigned idx = 0; idx < numUnrolledInstances; ++idx) {
+      auto offsets = delinearize(strides, idx);
+      auto offsetValues =
----------------
Nit: we mildly prefer using `auto` when the type is either very long or obvious from the context. In this function, types are almost never obvious and one has to look up the declarations of other functions.


================
Comment at: mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp:222
+
+    // 1. Compute the inbBoundsCondition in the current loops ivs + offset
+    // context.
----------------
Typo: inbBounds


================
Comment at: mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp:239
+      // 3.a. If in-bounds, progressively lower to a 1-D transfer read.
+      BlockBuilder(&ifOp.thenRegion().front(), Append())([&] {
+        Value loaded1D = load1DVector(majorIvsPlusOffsets);
----------------
Plz remind me if `Append()` appends to the end of the block or before the terminator


================
Comment at: mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp:257
+        if (options.unroll) {
+          loop_yield(Value{vector_insert(vector, result, majorIvs)});
+          return;
----------------
https://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor

Maybe just do `vector = vector_insert(vector, ...); loop_yield(vector);`, reads better IMO


================
Comment at: mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp:322
+
+    // 1. Compute the inbBoundsCondition in the current loops ivs + offset
+    // context.
----------------
inbBounds again


================
Comment at: mlir/lib/Dialect/Vector/VectorUtils.cpp:35
+    return 0;
+  int64_t res = 1;
+  for (auto b : basis)
----------------
std::accumulate(basis.begin(), basis.end(), 1, std::multiplies<int64_t>()); ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80100