This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Add abstract base class for header phi recipes (NFC).
ClosedPublic

Authored by fhahn on Dec 27 2021, 4:13 AM.

Details

Summary

Not all header phis widen the phi, e.g. like the new
VPCanonicalIVPHIRecipe in D113223. To let those recipes also inherit
from a phi-like base class, add a more generic VPHeaderPHIRecipe
abstract base class.

Diff Detail

Unit TestsFailed

Event Timeline

fhahn created this revision.Dec 27 2021, 4:13 AM
fhahn requested review of this revision.Dec 27 2021, 4:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2021, 4:13 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Dec 27 2021, 1:39 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
586

This could be applied independently.

llvm/lib/Transforms/Vectorize/VPlan.h
1063

A [pure virtual] base class

inductions[.]and

1069

Keep IncomingBlocks in VPWidenPHIRecipe?

1094

phi/select >> phi

1103

it one >> if one

1108

"if there is one" - getOperand(1) asserts that there is one...

1139

Indep, of this patch, but why not have one constructor:

VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr)
    : VPHeaderPHIRecipe(VPVWidenPHISC, VPWidenPHISC, Phi, Start) {}
fhahn updated this revision to Diff 396382.Dec 28 2021, 1:46 AM
fhahn marked 6 inline comments as done.

Address latest comments, thanks!

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

I'll push it as a separate commit

llvm/lib/Transforms/Vectorize/VPlan.h
1063

Adjusted, thanks!

1069

Sounds good, thanks! Let's keep the base class as lightweight as possible.

1108

Removed.

1139

Good point! I rather remove the (single?) users of the constructor without start value as follow-up

fhahn updated this revision to Diff 396383.Dec 28 2021, 1:52 AM

Add back missing VPWidenPHIRecipe constructor.

Ayal accepted this revision.Dec 28 2021, 2:37 AM

Looks good to me, with a last nit.

llvm/lib/Transforms/Vectorize/VPlan.h
1121

Leave getIncomingValue() in VPWidenPHIRecipe, for VPlan native? It seems to complement getIncomingBlock() and addIncoming(), and is synonymous with getOperand(),,,

This revision is now accepted and ready to land.Dec 28 2021, 2:37 AM
This revision was landed with ongoing or failed builds.Dec 28 2021, 6:38 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.Dec 28 2021, 6:40 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
1121

Thanks, I moved it back to VPWidenPHIRecipe in the committed version.