[PATCH] D144128: [SLP] Check with target before vectorizing GEP Indices
Jonas Paulsson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list