This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Add VPBasicBlock::phis() helper (NFC).
ClosedPublic

Authored by fhahn on Apr 8 2021, 6:30 AM.

Details

Summary

This patch introduces a helper to obtain an iterator range for the
PHI-like recipes in a block.

Diff Detail

Event Timeline

fhahn created this revision.Apr 8 2021, 6:30 AM
fhahn requested review of this revision.Apr 8 2021, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 6:30 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added a comment.Apr 27 2021, 12:43 AM

Looks good to me!

This is aligned with BasicBlock::phis(), with a simpler implementation based on finding FirstNonPHI. Number of phi's is expected to be limited, so running twice until FirstNonPHI instead of once should be tolerable.

Would be good to document some place that all phi recipes appear before any non-phi recipe in VPBasicBlock, as in BasicBlock. (This assumption predates this patch.)

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

worth adding getFirstNonPHI() and have phis() use it?

1483

worth having both these recipes inherit from a common base class, which represents all phi recipes, resulting in a single isa<> here?

once some of these header phi recipes are converted into VPInstructions, this may be more complicated.

fhahn updated this revision to Diff 340763.Apr 27 2021, 2:25 AM

Updates:

  1. add comment about PHI node order to VPBasicBlock
  2. use existing getFirstNonPhi() in phis()
  3. add VPrecipeBase::isPhi(), use it in getFirstNonPhi

Looks good to me!

This is aligned with BasicBlock::phis(), with a simpler implementation based on finding FirstNonPHI. Number of phi's is expected to be limited, so running twice until FirstNonPHI instead of once should be tolerable.

Would be good to document some place that all phi recipes appear before any non-phi recipe in VPBasicBlock, as in BasicBlock. (This assumption predates this patch.)

Thanks for taking a look! I added a sentence to the top-level comment for VPBasicBlock saying that all PHI-like recipes must come before non-phi recipes. I hope that's sufficient or were you thinking about a different place?

fhahn added inline comments.Apr 27 2021, 2:28 AM
llvm/lib/Transforms/Vectorize/VPlan.h
1477

There's already a getFirstNonPhi, which I originally missed. I updated the code to use it directly (and moved phis() to getFirstNonPhi.

1483

worth having both these recipes inherit from a common base class, which represents all phi recipes, resulting in a single isa<> here?

I'm not sure if there is much benefit at the moment, as I don't think the PHI-like recipes could really share much in the common base class?

I added a VPRecipeBase::isPhi() helper to directly check if a recipe is PHI-like, so we have a single place for such a check. I think that should give us the same convenience with respect to having a simple check for PHI-like recipes, but without having to add an extra level in the class hierarchy for now. WDYT?

fhahn updated this revision to Diff 341161.Apr 28 2021, 5:39 AM

Rebased so this now applies directly on top of main.

Ayal accepted this revision.May 2 2021, 10:18 AM

This is fine, thanks!

This revision is now accepted and ready to land.May 2 2021, 10:18 AM
This revision was landed with ongoing or failed builds.May 2 2021, 11:20 AM
This revision was automatically updated to reflect the committed changes.