Page MenuHomePhabricator

[VPlan] Add first VPlan version of sinkScalarOperands.
Needs ReviewPublic

Authored by fhahn on Apr 11 2021, 4:12 AM.

Details

Reviewers
Ayal
rengolin
gilr
Summary

This patch adds a first VPlan-based implementation of sinking of scalar
operands.

The current version traverse a VPlan once and processes all operands of
a predicated REPLICATE recipe. If one of those operands can be sunk,
it is moved to the block containing the predicated REPLICATE recipe.
Continue with processing the operands of the sunk recipe.

The initial version does not re-process candidates after other recipes
have been sunk. It also cannot partially sink induction increments at
the moment. The VPlan only contains WIDEN-INDUCTION recipes and if the
induction is used for example in a GEP, only the first lane is used and
in the lowered IR the adds for the other lanes can be sunk into the
predicated blocks.

Diff Detail

Event Timeline

fhahn created this revision.Apr 11 2021, 4:12 AM
fhahn requested review of this revision.Apr 11 2021, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 4:12 AM
Herald added a subscriber: vkmr. · View Herald Transcript
gilr added inline comments.Tue, Apr 20, 2:36 AM
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
129

use VPBB instead?

fhahn updated this revision to Diff 338869.Tue, Apr 20, 7:45 AM

Updated to use VPBB in the lambda, thanks!

I think the original code also had a bug that meant we would sink instructions that may read from memory, which would be incorrect if we move it across a instruction that may modify the read memory. I added a new test case and I think the original sinkScalarOperands gets this wrong as well.

I also put up D100857, which adjusts the logic in the legacy version of sinkScalarOperands to only handle the scalar parts of recipes that cannot be sunk by the Vplan-to-Vplan sinkScalarOperands. This requires a bit more code, so I put it up as a separate patch, but it could also be folded into this one.

Kazhuu added a subscriber: Kazhuu.Thu, Apr 29, 2:20 PM
gilr added a comment.Mon, May 3, 8:22 AM

Very nice step forward!!

llvm/lib/Transforms/Vectorize/VPlan.h
1279 ↗(On Diff #338869)

Allowing to reset IsPredicated seems a bit fragile, as alsoPack is computed on contruction based on it. Since this patch only requires setting IsPredicated, perhaps better to have setIsPredicated() { IsPredicated = true; }. Might be worth adding a comment here that setting IsPredicated late is consistent with alsoPack being initialized to false as all users of the sunk recipes reside within the replicated region so no packing is required.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
108

Traversing the VPlan in this case doesn't require RPOT, right? can be done with a more efficient recursive scan?
Simpler to first collect all seeds into a worklist, saving the need to make early increments? If so, can alternatively be done by RecipeBuilder during VPlan construction.

128

Should be return !UI || UI->getParent() != VPBB?

133

Would be good to leave a comment here explaining that setting IsPredicated is done in favor of the legacy sinkScalarOperands() which still follows.

fhahn updated this revision to Diff 343186.Wed, May 5, 2:00 PM

use depth-first instead of RPO, add comment for setting IsPredicated and assertion.

fhahn marked 2 inline comments as done.Wed, May 5, 2:03 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
1279 ↗(On Diff #338869)

Agreed! I added a comment and an assertion.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
108

That's a good point, the traversal order should not matter. I updated it to use depth-first.

Simpler to first collect all seeds into a worklist, saving the need to make early increments? If so, can alternatively be done by RecipeBuilder during VPlan construction.

It may be simpler, but would introduce coupling with the recipe builder and I am not sure that's desired at this point? It seems like it may limit composability of VPlan transforms (e.g. if another transform introduces new predicated recipes, it may have to update the list of seed instructions in the builder or somewhere else)

128

Yes I think so, great catch!

fhahn updated this revision to Diff 345576.Fri, May 14, 3:30 PM

Remove need for setIsPredicate() by instead considering the operands of sink candidates in PredBB in the worklist as well.