[PATCH] D74944: [LoopVectorize] Fix cost for calls to functions that have vector versions
Francesco Petrogalli via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 21 08:50:48 PST 2020
fpetrogalli added a comment.
Hi @nemanjai , thank you for pointing this out! I didn't realize that my code was creating this problem.
Kind regards,
Francesco
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3289
NeedToScalarize = true;
- if (!TLI || CI->isNoBuiltin() || VFDatabase::getMappings(*CI).empty())
+ bool NoMappingForVF = true;
+ auto VectorMappings = VFDatabase::getMappings(*CI);
----------------
I think that what you have implemented here is doing something very similar to what is already implemented the API provided by the `VFShape` class and the `VFDatabase`.
You could first build the VFShape you expect to have for `VF` and `CI` by invoking the `get` method at https://github.com/llvm/llvm-project/blob/f37e899fd73d1a8958246d761eeb306a8846e81a/llvm/include/llvm/Analysis/VectorUtils.h#L103:
```
VFShape Shape = VFShape::get(*CI, {VF, false} /* EC*/, false /*HasGlobalPred */);
```
This will create the "shape" that the vector version of the vector call associated with `CI` will have when trying to vectorize with a vectorization factor of `VF`.
Then, you can check whether that specific shape for `VF` is available by querying the `VFDatabase` with it (see https://github.com/llvm/llvm-project/blob/f37e899fd73d1a8958246d761eeb306a8846e81a/llvm/include/llvm/Analysis/VectorUtils.h#L242):
```
VFDatabase(*CI).getVectorizedFunction(Shape);
```
This call will return a `nullptr` if there is no vector function that realizes vectorization with `VF` lanes.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3295
+
+ if (!TLI || CI->isNoBuiltin() || VectorMappings.empty() || NoMappingForVF)
return Cost;
----------------
`VectorMappings.empty() || NoMappingForVF)`: something is redundant here. Surely if `VectorMappings.empty()` there is `NoMappingForVF`. At the same time, if there is `NoMappingForVF` we should not attempt to vectorize? Or am I missing something?
================
Comment at: llvm/test/Transforms/LoopVectorize/widened-massv-call.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -vector-library=MASSV -force-vector-interleave=1 \
+; RUN: -vectorizer-maximize-bandwidth -O2 -loop-vectorize \
----------------
The problem you have pointed out is not related to the use of any specific `-vector-library`, as the `VFDatabase` class is build around the `vector-function-abi-variant` attribute in IR (see https://llvm.org/docs/LangRef.html#call-site-attributes).
I am happy for you to keep this `MASSV`-specific test, but I think you should also add a test in which the problem is raised directly by a call that uses the `vector-function-abi-variant` attribute.
Also, I think that you should also add a test in which you use the attribute, and vectorization doesn't happen because there are some mappings in the attribute, but there is no mapping for the VF being checked.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74944/new/
https://reviews.llvm.org/D74944
More information about the llvm-commits
mailing list