Page MenuHomePhabricator

[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

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
1061–1062

Keep IncomingBlocks in VPWidenPHIRecipe?

1062

A [pure virtual] base class

inductions[.]and

1089–1090

phi/select >> phi

1099

it one >> if one

1104

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

1168

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
1061–1062

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

1062

Adjusted, thanks!

1104

Removed.

1168

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
1117

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
1117

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