Page MenuHomePhabricator

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

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

Details

Reviewers
Ayal
gilr
rengolin
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

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
7564

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
7564

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

7594

ditto?

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

Address comments, thanks!

fhahn added inline comments.Fri, Mar 3, 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
7564

Updated to print if no plan could be built.

7594

Thanks, added a debug print below.

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

This is fine, adding a minor nit.

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

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.Sun, Mar 19, 8:50 AM
fhahn updated this revision to Diff 506715.Mon, Mar 20, 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.Mon, Mar 20, 2:14 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8683

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

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

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
8683

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.Tue, Mar 21, 4:48 AM
fhahn marked an inline comment as done.

Rename to tryToBuildVPlanWithVPRecipes as suggested, thanks!

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

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.