This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Allow building a VPlan to may fail.
ClosedPublic

Authored by fhahn on Jan 30 2023, 5:16 AM.

Details

Summary

Update the planning code constructing VPlan to allow building VPlans to
fail. This allows us to gradually shift some legality checks to VPlan
construction. The first candidate is checking if all users of
first-order recurrence phis can be sunk past the recipe computing the
previous value.

Diff Detail

Unit TestsFailed

Event Timeline

fhahn created this revision.Jan 30 2023, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 5:16 AM
fhahn requested review of this revision.Jan 30 2023, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 5:16 AM
Ayal added a comment.Feb 1 2023, 9:15 AM

Adding a couple of nits. The optional functionality added cannot yet be exercised (with a test)?

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
352

What can be said about Range.End if this returns empty handed? As explained above, Range.End is possibly decreased according to the relevant VF's if this returns a VPlanPtr.

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

Worth printing/reporting something if no plan was built?

fhahn added a comment.Feb 4 2023, 1:54 PM

Adding a couple of nits. The optional functionality added cannot yet be exercised (with a test)?

Unfortunately I don't think it can be tested independently. D142886 starts exercising the code path.

Ayal added inline comments.Feb 9 2023, 3:38 PM
llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
352

Range.End must be greater than Range.Start in order to make forward progress even if a VPlan cannot be built for Range.Start. Suffice to say that if a VPlan cannot be built for Range.Start then Range.End must be set such that a VPlan cannot be built for any VF between Range.Start (inclusive) and Range.End (exclusive), where Range.End must be greater than Range.Start?

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

Above LLVM_DEBUG() probably prints nothing and no report is issued for ignoring UserVF if no plan was built?

7628

ditto?

fhahn updated this revision to Diff 502073.Mar 3 2023, 12:53 AM
fhahn marked 5 inline comments as done.

Address comments, thanks!

fhahn added inline comments.Mar 3 2023, 12:54 AM
llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
352

I updated the comment to try to make things clearer. If no VPlan can be built for a given range, the largest VF included in Range will be set to the maximum VF for which no plan could be built.

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

Updated to print if no plan could be built.

7628

Thanks, added a debug print below.

Ayal accepted this revision.Mar 19 2023, 8:50 AM

This is fine, adding a minor nit.

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

nit: why is std::move() needed here now, because buildVPlanWithVPRecipes() now returns an std::optional<VPlanPtr> instead of a VPlanPtr? Could nullptr be returned instead of optional?

This revision is now accepted and ready to land.Mar 19 2023, 8:50 AM
fhahn updated this revision to Diff 506715.Mar 20 2023, 2:13 PM

Thanks for taking a look! I think it would be good to land this together with a concrete use of the new functionality, which would be D142886 (still needing review :))

fhahn marked an inline comment as done.Mar 20 2023, 2:14 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8724

IIUC it's needed because std::unique_ptr doesn't allow copying.

Ayal added inline comments.Mar 21 2023, 1:27 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8724

Ok, but can an empty unique_ptr initialized to null be returned, instead of optional and move?

Now that building may fail, you could even call it tryBuildVPlanWithVPRecipes .

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

std::nullopt is the modern and saver version of nullptr. It is more obvious that std::nullopt denotes failure.

fhahn updated this revision to Diff 506918.Mar 21 2023, 4:48 AM
fhahn marked an inline comment as done.

Rename to tryToBuildVPlanWithVPRecipes as suggested, thanks!

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

We cannot avoid the move I think, regardless of whether std::optional is used or not. Without the move, std::unique_ptr's copy constructor would be called which is deleted.

I could change it to not use std::optional while retaining the move if you have a strong preference to avoid std::optional.

Rename to tryToBuildVPlanWithVPRecipes as suggested, thanks!

I was never questioning the move. I just wanted to note that I prefer std::nullopt over nullptr to denote failure.

Ayal added inline comments.Mar 23 2023, 7:15 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8724

No strong preference, patch was accepted as is, just wanted to clarify the options.
The optional is optional - had a standard pointer been returned rather than a unique_ptr, it would have been more natural to return null rather than optional, so it seems natural to also do so here and return an empty unique_ptr rather than wrap with optional.
(The move can be avoided, by always storing the VPlanPtr in VPlans, even when it is empty. But then one must check for empty VPlans whenever iterating over them.)

fhahn updated this revision to Diff 512389.Apr 11 2023, 3:32 AM

Rebase before landing.

This revision was landed with ongoing or failed builds.Apr 11 2023, 7:41 AM
This revision was automatically updated to reflect the committed changes.