[PATCH] D74944: [LoopVectorize] Fix cost for calls to functions that have vector versions

[Francesco Petrogalli via Phabricator via llvm-commits]

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