[PATCH] D96021: [LoopVectorize] NFC: Move UserVF feasibility checks to separate function.

Fri Mar 12 07:30:56 PST 2021

sdesmalen added a comment.

Hi @fhahn,

> [..]
> In the end, the code is not vastly different and I think there are no absolute answers here. I hope the above explains the direction from which I am looking at this a bit better than I did initially.

Yes, thanks for this, that was helpful! My original reason for splitting the patch series out like this was to make some trivial NFC "move code around" changes that were relatively simple to review, before making functional changes for scalable vectors. After seeing more clearly what you mean, I agree with you that the code becomes a bit more readable by being a bit more rigorous in the refactoring, and it also simplified the follow-up patches a bit.

Long story short, I have updated my patch series to align it more with the approach that you outlined in D96997 <https://reviews.llvm.org/D96997>. My new patch is in D98509 <https://reviews.llvm.org/D98509> and I'll abandon this series.

> Another thing I am not entirely sure is the reduction checks. For now it seems like for AArch64 the VF is not really used, so in the patch it just passes any scalable VF. not sure if that might change in the future, but I think I saw in one of the comments mentioning that we might to remove VFs after computing the max VF anyways

I initially thought we could discard individual VF candidates that don't match the "can this vectorize" criteria. This may actually be too fine grained because of how VFRange is used to specify a range of VFs, making it difficult to discard individual VFs as candidates. As you say, for SVE it doesn't look at the specific VF anyway. For now, I've added a FIXME around it, indicating this will change in the future.

> With that in mind, the UserVF handling should be really simple: we just have to check if it is within the upper bound. For scalable vectors, we also have the fallback. But this may just be temporary, because once we also pick a max scalable VF, we can instead proceed with ignoring the UserVF, same as we do for fixed vectors.

Yes, I have actually implemented that behaviour in the new patch. If a scalable UserVF is not valid, it should not fall back on a less-wide scalable VF, or a VF with the same minimum number of lanes as the UserVF and drop the 'scalable' flag, as it does now. It should instead ignore the UserVF, and do what it would otherwise do: pick a suitable VF that is most cost-effective.

Thanks again!

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

https://reviews.llvm.org/D96021