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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
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) {
----------------
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.


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

https://reviews.llvm.org/D111125