[VPlan] Allow building a VPlan to may fail.

[fhahn]

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.

[Ayal]

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]

In D142885#4097064, @Ayal wrote:

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]

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]

Address comments, thanks!

[fhahn]

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.

[fhahn]

ping :)

[Ayal]

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?

[fhahn]

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]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

8683

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

[Ayal]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

8683

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

[tschuett]

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]

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.

[tschuett]

In D142885#4209221, @fhahn wrote:

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.