Page MenuHomePhabricator

[VPlan] Move reduction start value creation to widenPHIRecipe.
ClosedPublic

Authored by fhahn on Wed, Jan 6, 6:40 AM.

Details

Summary

This was suggested to prepare for D93975.

By moving the start value creation to widenPHInstruction, we set the
stage to manage the start value directly in VPWidenPHIRecipe, which be
used subsequently to set the 'resume' value for reductions during
epilogue vectorization.

It also moves RdxDesc to the recipe, so we do not have to rely on Legal
to look it up later.

Diff Detail

Event Timeline

fhahn created this revision.Wed, Jan 6, 6:40 AM
fhahn requested review of this revision.Wed, Jan 6, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jan 6, 6:40 AM
Herald added a subscriber: vkmr. · View Herald Transcript
fhahn updated this revision to Diff 314884.Wed, Jan 6, 6:47 AM

Simplified StartV = Iden = StartV; to Iden = StartV, as suggested in D93975

Harbormaster completed remote builds in B84206: Diff 314884.
gilr accepted this revision.Fri, Jan 8, 8:40 AM

LGTM, with a nit.
(Seems reduction might be worth splitting widenPhiInstruction() and its own Recipe, but that's beyond the scope of this patch)
Thanks!

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

To be on the safe side I'd check here for Legal->isReductionVariable(P) and assert that RdxDesc and StartV are not null inside.

This revision is now accepted and ready to land.Fri, Jan 8, 8:40 AM
fhahn added a comment.Fri, Jan 8, 9:53 AM

LGTM, with a nit.
(Seems reduction might be worth splitting widenPhiInstruction() and its own Recipe, but that's beyond the scope of this patch)
Thanks!

Thanks Gil! Yes I think we can think about splitting them up as follow-ups.

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

I added an assertion that Legal->isReductionVariable(P) and StartV must be true. I also added another assertion below to ensure we do not miss legal induction variables for which RdxDesc is not set.