-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[LoopVectorize] Enable vectorisation of early exit loops with live-outs #120567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
cfc3c62
9432e60
cff198a
553bad4
71fec6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -500,8 +500,15 @@ void VPBasicBlock::execute(VPTransformState *State) { | |
UnreachableInst *Terminator = State->Builder.CreateUnreachable(); | ||
// Register NewBB in its loop. In innermost loops its the same for all | ||
// BB's. | ||
if (State->CurrentParentLoop) | ||
State->CurrentParentLoop->addBasicBlockToLoop(NewBB, *State->LI); | ||
Loop *ParentLoop = State->CurrentParentLoop; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to improve tracking the CurrentParentLoop instead of working around it here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible, although you'll still end up with similar looking code in VPlan::execute where we execute the blocks:
It's worth bearing in mind that in VPRegionBlock::execute we also set the ParentLoop:
and VPRegionBlock is included in that list. So depending upon how the blocks are traversed you may need to cache the previous value each time, i.e. something like:
I'm honestly not sure if that's any better? The only way to do this neatly I think is if you deal with the VP blocks with a single successor exit block in a different loop, i.e.
|
||
// If this block has a sole successor that is an exit block then it needs | ||
// adding to the same parent loop as the exit block. | ||
VPBlockBase *SuccVPBB = getSingleSuccessor(); | ||
if (SuccVPBB && State->Plan->isExitBlock(SuccVPBB)) | ||
ParentLoop = State->LI->getLoopFor( | ||
cast<VPIRBasicBlock>(SuccVPBB)->getIRBasicBlock()); | ||
if (ParentLoop) | ||
ParentLoop->addBasicBlockToLoop(NewBB, *State->LI); | ||
State->Builder.SetInsertPoint(Terminator); | ||
|
||
State->CFG.PrevBB = NewBB; | ||
|
@@ -949,6 +956,10 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV, | |
} | ||
} | ||
|
||
bool VPlan::isExitBlock(VPBlockBase *VPBB) { | ||
return isa<VPIRBasicBlock>(VPBB) && VPBB->getNumSuccessors() == 0; | ||
} | ||
|
||
/// Generate the code inside the preheader and body of the vectorized loop. | ||
/// Assumes a single pre-header basic-block was created for this. Introduce | ||
/// additional basic-blocks as needed, and fill them all. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't comment on the generated file, it looks like the size in pixels changed, just checking if that was intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't specifically try to change the size in pixels - I just ran
dot -Tpng ../llvm/docs/vplan-early-exit.dot
. Is there a recommended command for regenerating this? I couldn't see any documentation about this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that
dot -Tpng -Gsize=15,14\! -Gdpi=100 vplan-early-exit.dot -o vplan-ee.png
resulted in a larger image, though not the exact size I requested. May require some experimenting to get a good image. You could put the size and dpi directives in the .dot file instead to preserve it the next time someone modifies it.It seems graphviz isn't terribly cooperative about this, and you need other tools to rescale or pad the edges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I've regenerated it with that command and hopefully the image looks bigger now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't realize that there were different defaults on different platforms/version. The larger versions looks good, thanks!