[PATCH] D78071: [mlir] [VectorOps] Progressive lowering of vector.broadcast

[Aart Bik via Phabricator via llvm-commits]

aartbik added a comment.

Thank Nicolas. Good feedback!



================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:1028
+        auto pos = rewriter.getI64ArrayAttr(d);
+        result = rewriter.create<vector::InsertOp>(loc, dstVectorType, bcst,
+                                                   result, pos);
----------------
nicolasvasilache wrote:
> nit: we could add another builder in VectorOps to create the `ArrayAttr` for us and just use `d`.
We actually already have a convenience method for that, I just had to drop the redundant type for the destination.


================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:1051
+    assert(srcRank == dstRank);
+    for (int64_t r = 0; r < dstRank; r++) {
+      if (srcVectorType.getDimSize(r) != dstVectorType.getDimSize(r)) {
----------------
nicolasvasilache wrote:
> So this loop runs at most once but I got confused.
> Can we turn it into a find_if to get the first rank that does not match and save that.
> Early replace + exit if not found. then the rest will become easier to follow.
Yes, I had it first split in a search-loop followed by action code, but liked the concise version, but I can see why that is too confusing, so changed to search-loop again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78071