[llvm] [LV] Vectorize selecting last IV of min/max element. (PR #141431)

[Florian Hahn via llvm-commits]

fhahn wrote:

> #142322 is good and will definitely be useful in the future. However, I’m a bit confused about VPlanTransforms::legalizeUnclassifiedPhis. Do we have any plans to move legality checks into the VPlan transformation phase? What are the benefits of doing this? 
> 

We already do some legality checks in VPlan. The main one is for fixed-order recurrences, where we bail out if we fail to re-order the users of a recurrence. A number of VPlan transformations also check legality on VPlan before applying a transformation (including convertToSingleScalar). There are also some transforms that are required for legality, but work independently of LoopVectorizationLegality (e.g. dropPoisonGeneratingRecipes and the existing adjustRecipesForReductions, that converts recipes as needed for in-loop reductions).

I see the motivation similar to moving the cost model to be VPlan-based. Performing legality checks in VPlan allows combining checks directly with the transformations, which can be simpler/more direct due to not first requiring to collect the information separately in one place and then trying to translate it later to the already constructed VPlan based on IR references. It can also encourage more composable recipes. Many/most improvements over the last few years built on top of VPlan instead of the existing legacy code, and overall the approach has been very successful in my opinion.

In general, being able to perform legality checks directly on VPlan can also allow more flexibility, as we do not have to commit to a particular strategy (or check all possible strategies) early on (e.g. tail-folding vs. not folding the tail), and some legality issues can be resolved by other transforms that can be applied directly (e.g sinking/hoisting for fixed-order recurrences, peeling, distribution).

I think new capabilities serve as a great opportunity to improve and evolve the infrastructure to make sure the current VPlan representation is flexible and expressive enough to drive them

> I’m concerned that identifying min/max recurrences so late might limit future support for in-loop min/max reductions. Also, when min/max recurrences have LoopExitInstr, would this approach prevent us from using tail folding for vectorization?

In our current setup, the transform happens after adjustReciepsForReductions. I think we can’t easily create in-loop reductions after that as-is. But I think this is just a limitation of the current order. As follow-ups, we should be able to improve the order like this: 1) Add ComputeReductionResults and update exit users, 2) Legalize the remaining phis, and 3) Convert recipes for in-loop reductions.

As for tail folding with exit users, this won’t work with the current canFoldTailByMasking. But as I mentioned earlier, this could be a good chance to rethink/evolve how and when we commit to tail folding and check its legality.

> Separately, I submitted #141467 and #142335, but they haven’t received any review yet. I’d like to understand if there are any major design issues with my patches that are preventing them from being accepted.

Yep thanks! I originally shared the patch to get the ball rolling and to better understand if performing the checks directly based on VPlan would be feasible as an option, so we can consider the trade-offs more concretely. 

#141467 and #142335 fit into the current system. One potential drawback is that it requires carrying state collected in IVDescriptors via LoopVectorizationLegality to LoopVectorize, which adds complexity. For this particular case, there may still be some limitations to VPlan currently that make it desirable not to use a VPlan-first approach here, but as I mentioned earlier, it may serve as a good opportunity to evolve the infrastructure and pave the road for further modularization/VPlan-ization of more legality checks.

https://github.com/llvm/llvm-project/pull/141431