[PATCH] D67572: [VectorUtils] Introduce the Vector Function Database (VFDatabase).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D67572#1781286 <https://reviews.llvm.org/D67572#1781286>, @uabelho wrote:

> Hi,
>
> Thanks for the reply!
>
> Ok, I think I understand what is happening now at least.
>
> We have a bunch of target intrinsics that we say are vectorizable, but we don't provide a name of the vector version of the intrinsic.
>
> This meant that before this patch
>
>   LoopVectorizationLegality::canVectorizeInstrs()
>
>
> accepted to vectorize the loop since
>
>   TLI->isFunctionVectorizable(CI->getCalledFunction()->getName())
>   
>
> returned true for the intrinsic.
>
> Then in LoopVectorizationCostModel::getVectorCallCost we decided that the call to the intrinsic should be scalarized, since
>
>   TLI->isFunctionVectorizable(FnName, VF)
>   
>
> returned false.
>
> So the loop was vectorized, but we got VF calls to the scalar version of the intrinsic, just as we wanted.
>
> However, with this patch, the check in
>
>   LoopVectorizationLegality::canVectorizeInstrs()
>   
>
> now says false, since we do
>
>   VFDatabase::getMappings(*CI).empty()
>   
>
> and we indeed get empty mappings since we don't provide any vector version.
>
> So the presence of an intrinsic that we don't provide a vector version for prevents vectorization of the entire loop, even if it would we totally ok to do VF calls to the scalar version instead.
>
> Is this change in behavior intended?


This change in behavior sounds a bit worrying for down-stream targets.

I think we should have at least an assertion making sure the check in LoopVEctorizationLegality succeeds in all cases it did previously with isFunctionVectorizable, otherwise down-stream targets will silently miss out on vectorization. But I think ideally the patch would preserve the existing behavior in the cases @uabelho  described.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67572