[PATCH] D144128: [SLP] Check with target before vectorizing GEP Indices

Mon Mar 20 03:36:20 PDT 2023

fhahn added a comment.

In D144128#4202886 <https://reviews.llvm.org/D144128#4202886>, @jonpa wrote:

> Sorry about your regression. I looked for a runline in the "godbolt" link, but could not find one.

I think it is just `opt -passes=slp-vectorizer`.

> As I recall, there were the basic idea here to not look at GEPs in collectSeedInstructions() for a target that does like to vectorize address computations. On SystemZ, this would mean having to use an expensive vector element extract instruction, which is never really a good idea. Since the tuning of the vectorizers with cost functions is far from being perfect, I think it's good to add a broad general heuristic if possible to avoid doing "bad things".
>
> I added this check in collectSeedInstructions(), but during review it was later then also added in NotProfitableForVectorization(). I am curious if it helps your problem to remove the change in the latter function?
>
> This patch is not super important for SystemZ, but rather just "probably a good idea". Not much code change. It would be kind of ok to revert it if it really causes a regression. On the other hand, perhaps you should consider returning false from prefersVectorizedAddressing(), if you don't generally prefer this heuristic?

Thanks for elaborating the motivation!  IMO it would be slightly preferable to adjust the cost on SystemZ to reflect an accurate cost (and prevent SLP from using it if the whole tree isn't profitable over all). One issue with using `prefersVectorizedAddressing` seems to be that it indicates a preference (and is used as such in other uses), whereas here it is used to not even attempt using vector addresses if it is the only option. The AArch64 backend in general prefers non-vector addresses if possible, but they are costed relatively accurately and can be beneficial if the larger tree offsets the cost.

So unfortunately changing `prefersVectorizedAddressing` would potentially have undesirable side-effects. It might be better to revert the commit for now and then recommit it while avoiding the regressions. While I don't have any performance data to back it up I think that X86 would likely be impacted as well by a similar issue to AArch64.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144128