Page MenuHomePhabricator

[VPlan] Switch VPWidenRecipe to be a VPValue
ClosedPublic

Authored by dmgreen on Sep 28 2020, 2:04 PM.

Details

Summary

Similar to other patches like D84680, this makes VPWidenRecipe a VPValue. Because of the way it interacts with the reduction code it also slightly alters the way that VPValues are registered, removing the up front NeedDef and using getOrAddVPValue to create them on-demand if needed instead.

Diff Detail

Event Timeline

dmgreen created this revision.Sep 28 2020, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 2:04 PM
dmgreen requested review of this revision.Sep 28 2020, 2:04 PM
dmgreen updated this revision to Diff 297944.Oct 13 2020, 1:29 PM

Update and rebase. This still removes NeedDef and now also changes the way that some recipes are registered and removed, to allow the reductions to replace VPWidenRecipes.

fhahn requested changes to this revision.Mon, Nov 9, 4:46 AM

Thanks for putting up this patch. This is indeed a conversion that also needs to be done and for which I didn't put up patches in the series.

Some comments inline. I think this might also need a rebase.

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

WidenRecipe is always guaranteed to be a VPValue here because it is either a VPWidenSelect or VPWiden recipe, right? I think it should be possible to call toVPValue unconditionally, or better just cast<VPValue>(WidenRecipe), possible with a comment why it is safe?

8009

We also need to update widenInstruction to update State to register the result for the VPValue (replace VectorLoopValueMap.setVectorValue(&I, Part, V); with something like State.set(Def, &I, V, Part);, where Def is the VPValue of the widen recipe.

llvm/lib/Transforms/Vectorize/VPlan.cpp
890–893

I think this needs updating to print something like VPValue result = Opcode VPValue operands.

Should be possible by using printAsOperand(O, SlotTracker) for the VPValue result, Instruction::getOpcodeName for the opcode and printOperands for the operands.

This revision now requires changes to proceed.Mon, Nov 9, 4:46 AM
dmgreen updated this revision to Diff 305380.Sun, Nov 15, 10:40 AM
dmgreen marked an inline comment as done.

Rebase and address comments

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

Yeah this was written before VPWidenSelect went in.

This all relies on changing VPReductionRecipe first I think. As you can imagine, as these are already actually using VPValues, they become the most interrelated recipes to change.

fhahn accepted this revision.Sun, Nov 22, 6:26 AM

LGTM, thanks!

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

I think this is the only user of addOrReplaceVPValue. If so, it would be good to remove it.

7951

cast<VPValue>(RedRecipe) should not be needed, because RedRecipe is of type VPReductionRecipe, which in turn inherits from VPValue?

llvm/lib/Transforms/Vectorize/VPlan.cpp
890–893

Thanks! There's now a printOperands() function that could be used to print the operands here.

This revision is now accepted and ready to land.Sun, Nov 22, 6:26 AM

OK. Thanks for the review. Lets give this a go and see if anything complains. I've managed to run a sanitized build successfully at least, but let me know if anything else doesn't work.

This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Wed, Nov 25, 3:11 AM

OK. Thanks for the review. Lets give this a go and see if anything complains. I've managed to run a sanitized build successfully at least, but let me know if anything else doesn't work.

Thanks! I rebased my set of patches on top of it an everything seems to work out fine. One thing I forgot to mention was adding a VPReductionSC case to VPUser::classof, which I did in rGad5b83ddcf45, together with unifying the VPValue ID names (some were using VPVBla and other VPBla)