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

Fri Mar 17 11:27:06 PDT 2023

jonpa added a comment.

In D144128#4199494 <https://reviews.llvm.org/D144128#4199494>, @fhahn wrote:

> In D144128#4199448 <https://reviews.llvm.org/D144128#4199448>, @zjaffal wrote:
>
>> Hi this patch is causing some regressions. 
>> If you look at the example I attached the code sequence is no longer being vectorised when it is beneficial to do so.
>> https://godbolt.org/z/MTex1z73K
>>
>> Can you take a look please?
>
> To add to what @zjaffal said, I am curious why the current cost-modeling wouldn't be sufficient to prevent cases where vectorizing GEPs would be not profitable?
>
> The case @zjaffal shared requires a more costly vector GEP, but the cost is offset by the benefits from vectorizing the rest of the tree.

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

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144128