[PATCH] D142885: [VPlan] Allow building a VPlan to may fail.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8823
+    if (auto Plan = buildVPlanWithVPRecipes(SubRange, DeadInstructions))
+      VPlans.push_back(std::move(*Plan));
     VF = SubRange.End;
----------------
fhahn wrote:
> tschuett wrote:
> > Ayal wrote:
> > > fhahn wrote:
> > > > Ayal wrote:
> > > > > 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`?
> > > > IIUC it's needed because `std::unique_ptr` doesn't allow copying.
> > > Ok, but can an empty unique_ptr initialized to null be returned, instead of optional and move?
> > std::nullopt is the modern and saver version of nullptr. It is more obvious that std::nullopt denotes failure.
> 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`.
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142885/new/

https://reviews.llvm.org/D142885