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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai marked 2 inline comments as done.
nemanjai added inline comments.


================
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);
----------------
fpetrogalli wrote:
> 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.
> 
Ah, cool. I didn't really look at the interface very much. Thanks for the suggestion.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3295
+
+  if (!TLI || CI->isNoBuiltin() || VectorMappings.empty() || NoMappingForVF)
     return Cost;
----------------
fpetrogalli wrote:
> `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?
Right, the cost we return in this block includes the scalarization cost, so if `NoMappingForVF` is `true`, we won't try to vectorize. I suppose we could return some extremely high cost in this case that is guaranteed not to allow vectorization.

I just implemented it this way because this just gets us back to the behaviour before the commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74944