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

Stanislav Mekhanoshin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 24 10:41:43 PST 2020

rampitec added a comment.

In D82227#2413359 <https://reviews.llvm.org/D82227#2413359>, @jonpa wrote:

> 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...

WRT the testcase, it is really the testcase in the patch with a single modification right before ret void:

  store float %phi31, float* undef
  ret void

That makes the value not dead and then if I feed opt's output into llc I either get spilling with wide vectors (first case) or not (second case):

  opt -slp-vectorizer -slp-max-reg-size=1024 -S < slp-max-phi-size.ll | ~/work/llvm/rel/bin/llc -march=amdgcn -mcpu=gfx900
  opt -slp-vectorizer -slp-max-reg-size=256 -S < slp-max-phi-size.ll | ~/work/llvm/rel/bin/llc -march=amdgcn -mcpu=gfx900

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list