[PATCH] D102394: [LoopVectorize] Don't attempt to widen certain calls for scalable vectors

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1708
+bool AArch64TTIImpl::isLegalScalableVectorIntrinsic(Intrinsic::ID IID) const {
+  switch (IID) {
+  case Intrinsic::sin:
----------------
david-arm wrote:
> sdesmalen wrote:
> > I'd rather write this the other way around, have a list here of intrinsics we //do// support and can safely return 'true' for, and fall back on BaseT's implementation for the 'default' case (which seems to be missing here), which is more pessimistic and only returns true if it knows the intrinsics can always be vectorized, i.e. the trivially vectorizable cases.
> So we do fall back on the BaseT version, it's just I didn't do it explicitly in the `default:` case. For some reason I thought this coding structure was the preferred way - I'm never really too sure about the coding style of switch statements in LLVM!
> 
> It's also worth pointing out that that the BaseT version calls isTriviallyVectorizable where even `sin` and `cos` will return true. Therefore, returning the BaseT version for those cases is not what we want. I could either:
> 
> 1. Fill in the complete set of entries here with both the false and true cases, or
> 2. Change the BaseT version to always return false.
> 
> Any preferences?
> It's also worth pointing out that that the BaseT version calls isTriviallyVectorizable where even sin and cos will return true. Therefore, returning the BaseT version for those cases is not what we want.

That suggests to me that the BaseT implementation for isLegalScalableVectorIntrinsic is too lenient and shouldn't rely on isTriviallyVectorizable. Returning 'false' seems like a better starting point.


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

https://reviews.llvm.org/D102394