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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

In D74944#1889360 <https://reviews.llvm.org/D74944#1889360>, @nemanjai wrote:

> In D74944#1887086 <https://reviews.llvm.org/D74944#1887086>, @fpetrogalli wrote:
>
> > Hi @nemanjai ,
> >
> > thank you for updating the code.
> >
> > Sorry for being picky, I think you should add another test. Your code need to work also for the `vector-function-avi-variant` attribute and not just for the `-vector-library=` option.
> >
> > Kind regards,
> >
> > Francesco
>
>
> Will do. Note also that this will be the only such test case as far as I can tell, so we might want to add some more coverage there as well independent of this patch.


Yes, you are right. I'll prepare a patch that does vectorization without using the pre-built TLI infos, just the attribute in the IR.

Thank you for your work!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3283
+
+  if (!TLI || CI->isNoBuiltin() || !VecFunc)
     return Cost;
----------------
Please correct me if I am wrong, but it seems to me that the situation `!VecFunc` when `VecFunc == nullptr` is not checked by the tests you have in this patch.

I believe that such test can be done by using the same IR as in `llvm/test/Transforms/LoopVectorize/widened-massv-vfabi-attr.ll`, with the attribute string being replaced by a 4 lane mangled name `_ZGV_LLVM_N4v_llvm.sin.f64(whatever)`.

Could you please add such test?

Regarding the following: 

> Can you please justify? I think the idea is that if we didn't have a TLI object, we would end up with nullptr from getVectorizedFunction() but want to make sure.

The TLI is not needed anymore in the LoopVectorizer to check for vectorized function, because all infos needed are carried by the `VFDatabase`. The TLI is a requirement of the `InjectTLIMappings` pass, which populate the IR with the `vector-function-abi-variant` attribute that are constructed out of the ones stored in the TLI. By explaining this, I just realized that removing the `!TLI` from inside the vectorizer might require a separate patch, so I am happy for you to keep it in here. Sorry for the noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74944