[PATCH] D78071: [mlir] [VectorOps] Progressive lowering of vector.broadcast
Aart Bik via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 16 15:04:56 PDT 2020
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
More information about the llvm-commits
mailing list