Page MenuHomePhabricator

[VPlan] Use region for each loop in native path.
ClosedPublic

Authored by fhahn on Apr 3 2022, 1:34 PM.

Details

Summary

This patch updates the VPlan native path to use VPRegionBlocks for all
loops in a loop nest. Up to now, only the outermost loop used a region.

This is a step towards unifying both paths and keep things consistent
between them. It also prepares various code-gen parts for modeling the
pre-header in the inner loop vectorizer (D121624).

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Ayal added inline comments.Apr 27 2022, 3:27 PM
llvm/lib/Transforms/Vectorize/VPlan.cpp
267–268

Comment still intact?

279

remove brackets

279–285

} else if {

287

(unrelated to this patch): assert PredVPSuccessors[idx] == this?

291

worth a comment?
In this case PredVPBB is the exit block of a loop region - PredVPSuccessors.size() is zero?

294

error messages? cast instead of dyn_cast?
assert PredVPSuccessors is empty and PredBBTerminator has no successors?
assert PredVPBB == Reg->getExitBasicBlock()?
Reg>>LoopRegion, as Reg may also refer to Register(?)

307

Worth a comment - why have a(n exit) block with recipes if it's execute is skipped?
This is not instead of the continues in RPOT traversal below, right?

312

"NonReplicateRegion" >> "LoopRegion"?

313

Augment explanation of case B above. The case of SingleHPred being a LoopRegion is clear, what is the other case of SingleHPred residing in same LoopRegion enclosing this VPBB?

Need the extra brackets around

SingleHPred->getParent() == GetEnclosingNonReplicateRegion(this) &&
!IsNonReplicateR(SingleHPred)?

?

319

ditto

949–950

Explanation why if entry is a VPBB (preheader?) then VectorHeaderBB is merged into its predecessor - VectorPreHeader?

950

nit: why is setting CFG.PrevBB moved?

llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
76–78

typo: TheLoop

86

\p HeaderBB should be \p LatchBB in order to skip the backedge when it is a predecessor?

280

Why move?
Comment no longer true - Preheader's predecessors no longer set during RPO traversal?

315

Why move?

371

Why added spaces?

390

Backedge was not connected above?

fhahn updated this revision to Diff 426058.Apr 29 2022, 7:48 AM
fhahn marked 22 inline comments as done.

Rebased and addressed comments, thanks!

fhahn added inline comments.Apr 29 2022, 7:51 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8678–8680

Should be fixed!

llvm/lib/Transforms/Vectorize/VPlan.cpp
267–268

This is not relevant any longer, removed!

275–278

Unfortunately the diff was showing changes that were landed as part of a different review. I applied the suggestions in e66127e69bfa. That applies for all suggestions for changes in VPlan.cpp.

279–285

Simplified, thanks!

291

Added a comment, thanks!

294

Renamed and added extra assertions, thanks!

307

After re-ordering the patches, this is not needed any longer (originally this patch was placed before introducing any recipes in the exit)

312

This has been simplified in the latest version, now there's just a single IsLoopRegion helper.

313

Should be updated in the latest version

949–950

This is also gone in the latest version.

950

Not moved in the latest version.

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

Should be fixed, thanks!

2667

This now just does cast<VPRegionBlock>(getEntry()->getSingleSuccessor())

llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
76–78

I *think* this should be fixed.

86

HeaderBB/LatchBB is gone in the latest version.

280

Why move?

The move was accidental, but the comment is not longer true because the predecessor doesn't have any predecessors.

315

Undone.

371

Should be removed.

390

In the latest version, the backedge is connect earlier and removed here.

fhahn added a comment.May 29 2022, 4:33 AM

Ping :) I think all outstanding comments should have been addressed

Ayal added inline comments.May 29 2022, 3:11 PM
llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
90

Is this change needed?

110–113

Also comment that if \p BB belongs to a loop (except for preheader - said loop needs to be in the scope of VPlan?), then the new empty VPBB is placed in a corresponding VPRegion - if exists otherwise in a new one.

120

Deserves a separate getOrCreate[EnclosingLoop]Region()?

135

Would be good to add a test with nested loops showing how multiple regions are now being created, whereas until now only the outermost loop was region'd.

266–267

PreheaderVPBB >> VPlanPreheaderVPBB, to emphasize it's VPlan's outer/entry VPBB, to be returned at the end?

276

Is this change needed?

Move the comment about setting Preheader's predecessors along with setOneSuccessor? BTW, comment probably meant to refer to setting the predecessor(s) of Preheader's successor, as Preheader surely has no predecessors set, moreover during RPO traversal below.

324
  1. >> 2. ?
326

createVPInstructionsForVPBB() is removed because exit BB should not be populated; is the current population simply redundant, and can be removed independent of this patch? Should LoopExitVPBB, not being part of the (the) loop, be replaced by LiveOut VPUsers (surely independent of this patch)?

370
  1. >> 3. ?
386

HeaderPred >> PreheaderVPBB?

388

Unrelated to this patch: setName under LLVM_DEBUG?

390

getVectorLoopRegion()?

396

LatchSucc >> ExitVPBB? Where LatchVPBB == ExitingVPBB.

404
  1. >> 4.
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
27–30

Simply

ReversePostOrderTraversal<VPBlockBase *> RPOT(Plan->getEntry());

would do, or is the Wrapper needed for blocksOnly()?
Until now it works w/o the blocksOnly<VPBasicBlock>(), because TopRegion is skipped, and there were no other regions.

So now we do widen instructions in pre-header and (empty) exit/middle blocks having zero predecessors and successors respectively, right?

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
53

(To be renamed ExitingBasicBlock, which has no successors but a CondBit to denote when to exit the loop...)

llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp
49–51

Can explain:

// Check that the region following the preheader is a single basic-block region (loop).
178–180

ditto, outline to a common function checking for a single basic-block region (loop)?

fhahn updated this revision to Diff 432970.Mon, May 30, 12:23 PM
fhahn marked 17 inline comments as done.

Address latest comments, thanks!

llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
90

Nope, removed the stray change, thanks!

110–113

Extended the comment, thanks!

120

Given that is is just used here I'm not sure if it is worth pulling this out. Could be a lambda if you think early exits would help readability?

135

Good point, looks like we are missing a VPlan printing ttest for outer loop vectorization. Added in b7d2b160c3ba

266–267

Done, thanks!

276

Is this change needed?

It's not needed in the latest version of the patch, changed removed, thanks!

Move the comment about setting Preheader's predecessors along with setOneSuccessor? BTW, comment probably meant to refer to setting the predecessor(s) of Preheader's successor, as Preheader surely has no predecessors set, moreover during RPO traversal below.

Yeah, I'll remove the commit.

324

Numbering updated, thanks!

370

Numbering updated, thanks!

386

Fixed, thanks!

388

Will do separately.

390

updated, thanks!

396

Done! Also Updated LatchVPBB -> ExitingVPBB, with an assertion that getLoopLatch() == getExitingBlock()

404

Numbering fixed, thanks!

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
27–30

Simply
ReversePostOrderTraversal<VPBlockBase *> RPOT(Plan->getEntry());

Yes, updated, thanks!

So now we do widen instructions in pre-header and (empty) exit/middle blocks having zero predecessors and successors respectively, right?

They will be empty at the moment so nothing happens at the moment.

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
53

Updated!

llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp
49–51

Added, thanks!

178–180

Added, thanks!

fhahn updated this revision to Diff 432987.Mon, May 30, 2:14 PM

Remove some more dead code (VPBBsToFix is not needed any longer).

Ayal accepted this revision.Tue, May 31, 1:53 PM

This looks good to me, ship it!

llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
120

Added comment suffices to help readability, thanks!

266–267

nit: could alternatively use the "The" prefix to denote elements of the topmost/outer loop/region, following TheLoop: ThePreheaderBB, ThePreheaderVPBB, ...

This revision is now accepted and ready to land.Tue, May 31, 1:53 PM
This revision was landed with ongoing or failed builds.Wed, Jun 1, 2:41 AM
This revision was automatically updated to reflect the committed changes.