This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [VectorOps] Progressive lowering of vector.broadcast
ClosedPublic

Authored by aartbik on Apr 13 2020, 6:54 PM.

Details

Summary

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.

Diff Detail

Event Timeline

aartbik created this revision.Apr 13 2020, 6:54 PM

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.
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.

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.
Can we make the flow as follows:

if (r == 0) {
  extract + broadcast.+ loop (insert)
  replace and return 
}

loop(extract + broadcast + insert)
replace ans return
nicolasvasilache requested changes to this revision.Apr 16 2020, 12:49 PM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Vector/VectorTransforms.cpp
1076

In the proposed control-flow change, this line and the next would be duplicated.
You can reduce that with another nit: builder that takes an int.

This revision now requires changes to proceed.Apr 16 2020, 12:49 PM
aartbik updated this revision to Diff 258181.Apr 16 2020, 2:51 PM
aartbik marked 8 inline comments as done.

addressed ntv comments

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.

nicolasvasilache accepted this revision.Apr 16 2020, 2:58 PM
nicolasvasilache added inline comments.
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 :)
Move above to an early exit?

This revision is now accepted and ready to land.Apr 16 2020, 2:58 PM
aartbik updated this revision to Diff 258194.Apr 16 2020, 4:06 PM
aartbik marked 3 inline comments as done.

second round of suggestions addressed

This revision was automatically updated to reflect the committed changes.