Rather than having a full, recursive, lowering of vector.broadcast
to LLVM IR, it is much more elegant to have a progressive lowering
of each vector.broadcast into a lower dimensional vector.broadcast,
until only elementary vector operations remain. This results
in more elegant, step-wise code, that is easier to understand.
Also makes some optimizations in the generated code.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Nicolas,
Any comments? You were the main requester of replacing the recursion in favor of progressive lowering?
Aart
Sorry for the delay, I had started but got lost in the stretch logic and had to context switch.
So after looking deeper I think the code structure can be reworked to make it lest nesty and easier to follow (+ comments around each return point).
If you follow the suggestion you should end up with 3 duplicated lines for the loop (insert) part (see comments).
Given how much more readable I expect this to end up I'd say it's a good tradeoff.
mlir/lib/Dialect/Vector/VectorTransforms.cpp | ||
---|---|---|
1021 | nit: we could add another builder in VectorOps to create the ArrayAttr for us and just use d. | |
1044 | So this loop runs at most once but I got confused. | |
1046 | Please add a comment that this is the scalar (as vector<1x>) to vector case | |
1048 | same nit re build | |
1061 | I find the logic intertwining here quite tricky to follow. if (r == 0) { extract + broadcast.+ loop (insert) replace and return } loop(extract + broadcast + insert) replace ans return |
mlir/lib/Dialect/Vector/VectorTransforms.cpp | ||
---|---|---|
1076 | In the proposed control-flow change, this line and the next would be duplicated. |
Thank Nicolas. Good feedback!
mlir/lib/Dialect/Vector/VectorTransforms.cpp | ||
---|---|---|
1021 | We actually already have a convenience method for that, I just had to drop the redundant type for the destination. | |
1044 | 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. |
mlir/lib/Dialect/Vector/VectorTransforms.cpp | ||
---|---|---|
1021 | nit: extra braces | |
1052 | // All trailing dimensions are the same. Simply pass through. if (m < 0) { rewriter.replaceOp(op, op.source()); return success(); } and remove nesting for another level? | |
1084 | by the time I get here I forget why I am here in the first place :) |
nit: we could add another builder in VectorOps to create the ArrayAttr for us and just use d.