[PATCH] D82227: SLP: honor requested max vector size merging PHIs

Tue Nov 24 03:10:07 PST 2020

jonpa added a comment.

In D82227#2412089 <https://reviews.llvm.org/D82227#2412089>, @rampitec wrote:

> In D82227#2409429 <https://reviews.llvm.org/D82227#2409429>, @jonpa wrote:
>
>>> If for some reason a vector register width is not good enough driver for the vectorization I would rather create yet another target callback. It just happened that it is register width is currently used across the llvm to control it, but we can change it.
>>
>> It seems like we would want to retry vectorization from the next instruction in the group if the current group failed. It also seems like we want to try greater VFs first. This seems to be what tryToVectorizeList() is doing when it gets all of the group as input (like before this patch). This patch breaks both of these points: it retrys by skipping the *whole* failed group so for instance with a group of 5 PHIs where the first one is different, it will not restart on PHI#1, but on PHI#2, which is bad. It also forces a small VF which seems to miss vectorization opportunities.
>>
>> Instead of just updating this patch with a new hook would it perhaps be even better to put that hook inside tryToVectorizeList()? I see MinVF and MaxVF being set there and maybe that's a better place to cap VF? That way you would get the restart on the next instruction... Otherwise I agree it would be a good start to just use a new hook in the same place to get the old behavior back on SystemZ and other targets.
>
> That makes sense to me. I.e. try to restrart vectorization in tryToVectorizeList() but still make sure we do not produce vectors wider than requested by TTI.

Would you like to give that a try? I think this patch could basically be reverted, and then some new hook would be needed to control MinVF/MaxVF in tryToVectorizeList(). Personally, I think it could be nice with something like TTI->getMaximumVF()  or even getMaximumSLPVF()... Probably also good if with a test case for your target if possible that is supposed to not produce any spilling...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82227