[PATCH] D142015: [LV] Plan with and without FoldTailByMasking

Wed Feb 8 14:50:30 PST 2023

fhahn added a comment.

In D142015#4090401 <https://reviews.llvm.org/D142015#4090401>, @dmgreen wrote:

> In D142015#4084191 <https://reviews.llvm.org/D142015#4084191>, @fhahn wrote:
>
>> I think the overall direction is very desirable! However I am not sure if adding a FoldTail argument to the cost functions is an ideal way to get there as it requires a large number of changes and is not really scalable (e.g. adding support to generate a 3rd variant of plans may mean adding another argument to all functions)
>>
>> I am not sure if I am missing something, but would it be possible to instead run the planner multiple times with different cost-model configurations? I tried to sketch an option for more general support for that in D142669 <https://reviews.llvm.org/D142669>. Then we could generate the plans for tail-folding by just instantiating another instance of the cost model, sketched in D142670 <https://reviews.llvm.org/D142670>. Maybe that could help to simplify the patch?
>
> Multiple cost models was something I considered, but dismissed fairly quickly. I didn't think it would be something that anyone would agree to in a review - that we invent a new hierarchy of multiple cost-models each containing multiple vplans. The vplans should be flatter than that, and there is more in the cost-model that isn't dependent on FoldTail than is. I think we should be trying to reduce the number of cost-models, not increase it! The same argument about new variants could equally apply, just to an explosion in the number of cost-models needed.

IIUC we in practice already have multiple cost models (e.g. tail folded vs regular), but at the moment we effectively pick which one to run very early. So I don't think this adds a new hierarchy, but I might be missing some aspects you are thinking about here?

I agree that we shouldn't have a new hierarchies containing multiple plans. IIUC this is referring to D142669 <https://reviews.llvm.org/D142669> where it picks selects the list of plans for the most profitable VF returned by `plan`. This was mostly to keep the sketch of supporting multiple cost models simple (in terms of time to implement).  I *think* this could be improved by computing the cost for each plan directly, instead of doing so after constructing all plans in `selectVectorizationFactor`. I'll try to see if there are any stumbling blocks down this path.

I was also hoping to untangle some of the cost model functionality that computes the max VF, but unfortunately things are very tied together and it is not really feasible to split those parts of from computing costs per VF.

> This patch only pushed the FoldTailByMasking to the places that need it, just treating FoldTailByMasking like the VF already is. How married are you to the idea of multiple cost models? (Or how anti this are you?) I know this patch is fairly large but it feels like a cleaner end result once it is done. Let me know and I can try and get everything working the other way if necessary.

The changes are mostly mechanical, but it makes the mappings in the cost model more complicated and adding the additional argument potentially increases maintenance cost and makes the signatures slightly more complex. I just want to make sure we the considered potential tradeoffs/alternatives if we go down that route.

>> Another thing I noticed that the patch changes skeleton creation to take a VPlan as argument. I think it would be preferable to do this the other way around if possible, so instead model the difference between plans with and without tail-folding explicitly in the pre-header blocks in the VPlan. I've not looked at the details here to see how difficult that would be yet unfortunately though. But more than happy to iterate on that aspect together!
>
> I was hoping for one step at a time. Firstly we can break the dependency on specifying FoldTailByMasking early in the costmodel, whilst keeping then number of other changes needed minimal. Bigger changes can come later, but might require more changes. (It is useful, for example, to know that a plan is FoldTailByMasking when picking epilog plans).

Agreed, I think there are existing places where it would be convenient to know if the plan is FoldTailByMasking. One example is `adjustRecipesForReductions`, where this is the main thing for removing the reliance on the cost-model there.

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

https://reviews.llvm.org/D142015