[PATCH] D111125: [NFC][LoopVectorize] Remove setBestPlan in favour of getBestPlanFor

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

In D111125#3055328 <https://reviews.llvm.org/D111125#3055328>, @sdesmalen wrote:

> (Sorry for the slow reply, I was traveling last week)
>
> My suggestion on the other patch was to make the code structurally simpler. Removing plans prematurely complicates things, because you need to carefully keep track which plans can be removed and which ones may be needed.
> @bmahjour Is there a real problem with memory-usage that requires us to reduce the lifetime of these plans a little bit longer? If not, I'd like to suggest not removing the plans to keep the code simpler.

I do not know of a "real problem", but would expect loops with large bodies would benefit from some level of garbage collection. I've proposed to delete plans at specific static locations in the code when we know we don't need them, so should not need extra book keeping. I think we are very close to a solution that deletes unused plans, but if there are further complications or controversies I'm ok with dealing with the garbage collection aspect as a separate patch.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:315
+  /// Remove all VPlans except those containing one of the \p VFs.
+  void removePlansExcept(const ArrayRef<ElementCount> VFs) {
+    erase_if(VPlans, [VFs](const VPlanPtr &Plan) {
----------------
david-arm wrote:
> bmahjour wrote:
> > david-arm wrote:
> > > bmahjour wrote:
> > > > Instead of passing the list of VFs, why not pass in a `VPlanPtr` objects that's returned by `getBestPlanFor`? That would simplify the interface (since you don't have to pass in a list), simplifies the logic (since it makes it clear that we only delete plans that are not considered "best"), and avoids iterating through the list of VFs and VPlans a second time.
> > > I could do that, but would have to change this immediately in D109432 to accept a list of vplans, since in that patch different VFs will be in different plans. I would like to keep this as a list if possible. If you still think it's better to pass in a list of VPlanPtrs here instead of a list of VFs I can see if that makes the code simpler?
> > Ok, fair enough....I guess you could make it a list in this patch and pass a list of size one (just like you're doing now), but I still think that the type of the list element should be VPlanPtr.
> Hi @bmahjour, there is just one problem with your approach - the VPlanPtr is defined as std::unique_ptr<VPlan>, which means that I can only return a reference from getBestPlanFor. In order to pass a list of VPlanPtrs to removePlansExcept I have to first call getBestPlanFor to get VPlanPtr references. However, those references become invalid after calling removePlansExcept! That means that in effect I have to perform this call sequence:
> 
>   const VPlanPtr &PB1 = getBestPlanFor();
>   const VPlanPtr &PB2 = getBestPlanFor();
>   removePlansExcept({PB1, PB2});
>   const VPlanPtr &P1 = getBestPlanFor();
>   const VPlanPtr &P2 = getBestPlanFor();
> 
> This was the actual reason I passed in a list of VFs here instead. In order to pass in a list of VPlanPtrs I can think of three choices:
> 
> 1. We change VPlanPtr to be simply "typedef VPlan *VPlanPtr;" and stop making it unique. This allows the VPlanPtr to be copied. Then I change getBestPlanFor to return `const VPlanPtr` instead of a reference, which allows me to pass a list of VPlanPtrs to removePlansExcept.
> 2. We don't remove the plans at all.
> 3. We stick with passing a list of VFs to removePlansExcept.
> 
> I'm also happy to listen to alternative suggestions! However, at the moment I can't really move forward with this patch.
Hi @david-arm , sorry I was away last week. Yes, the references would indeed be invalidated as the container elements are moved around. However, the problem can be avoided by making `getBestPlanFor` and `executePlan` work with raw pointers. These functions simply use the passed vplan and do not alter it nor do they care about its ownership, so no need to pass them a smart pointer. Here's a patch I tried on top of yours that works fine:{F19690766}


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

https://reviews.llvm.org/D111125