Page MenuHomePhabricator

[VPlan] Turn VPReductionRecipe into a VPValue
ClosedPublic

Authored by dmgreen on Sep 27 2020, 11:53 AM.

Details

Summary

This converts the VPReductionRecipe into a VPValue, like other VPRecipe's in preparation for traversing def-use chains.

It doesn't yet change how the VPReductionRecipes are created. It will need to call replaceAllUsesWith from the original recipe they replace, but that is not done yet as VPWidenRecipe need to be created first.

Diff Detail

Event Timeline

dmgreen created this revision.Sep 27 2020, 11:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2020, 11:53 AM
dmgreen requested review of this revision.Sep 27 2020, 11:53 AM
fhahn added inline comments.Oct 5 2020, 8:40 AM
llvm/lib/Transforms/Vectorize/VPlan.cpp
941–942

now that this is a VPValue we should probably just reference the VPValue name using printAsOperand(), which will use the underlying value if available.

dmgreen updated this revision to Diff 296381.EditedOct 6 2020, 1:57 AM

Thanks. I've updated the printing, but not rebased this onto any other patches yet.

dmgreen updated this revision to Diff 297943.Oct 13 2020, 1:23 PM
dmgreen edited the summary of this revision. (Show Details)

Rebase and update.

fhahn added inline comments.Sun, Nov 15, 11:33 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8097

nit: assign in if() and use below?

8121

The recipes that get converted should also be updated to use State directly to map the result VPValue to the generated IR. Could this just be State.set(this, NextInChain, Part)?

llvm/lib/Transforms/Vectorize/VPlan.cpp
951

If it's not too much trouble, it would also be good to add a test to vplan-printing.ll

dmgreen updated this revision to Diff 305389.Sun, Nov 15, 2:12 PM

Ooops. I thought this was already using State.set, but uploaded the wrong version. Now with that and the other changes.

fhahn accepted this revision.Wed, Nov 18, 1:47 AM

LGTM, thanks!

This revision is now accepted and ready to land.Wed, Nov 18, 1:47 AM
fhahn added inline comments.Mon, Nov 23, 2:54 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7978

Now that both VPWidenREcipe and VPRedcutionRecipe are VPValues, we have to update all users of WidenRecipe with RedRecipe.

7986

We need to update the users of VPWidenRecipe's VPValue or assert that there are none.

dmgreen added inline comments.Tue, Nov 24, 6:37 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7978

I think these were happening in the other order, if I am understanding you correctly. The VPWidenRecipe patch is added on top of this, and hopefully fixes up what you are referring to?

7986

Hmm. This is the cmp in a min/max pattern. I am hoping that it would only have one user that we have removed above. At least, I can't seem to get it to vectorize when it has multiple uses. I'll make sure this is an assert in the VPWiden patch.

fhahn added a comment.Tue, Nov 24, 6:50 AM

small nit for the commit message: might be good to mention that this also turns it into a VPUser.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7978

Yes I think that should be fine after checking the other patch. Looks like I looked at the patches out-of-order for the final round.

7986

Sounds good, Could you perhaps add an assert that the users are empty to D88447?

fhahn added inline comments.Tue, Nov 24, 6:51 AM
llvm/lib/Transforms/Vectorize/VPlan.cpp
111

now that VPReductionRecipe is also a VPUser, it should also be added here. (this will go away once all recipes have been converted)

dmgreen added inline comments.Wed, Nov 25, 12:19 AM
llvm/lib/Transforms/Vectorize/VPlan.cpp
111

Yeah thanks, I noticed this one as I was getting the patch together for committing. I was trying a sanitizer build to check they are OK, which is taking a rather long time..

This revision was automatically updated to reflect the committed changes.