[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:
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5646
 
+bool LoopVectorizationCostModel::isScalableLoopLegal(ElementCount VF,
+                                                     StringRef &Msg) const {
----------------
MaxVF ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5646
 
+bool LoopVectorizationCostModel::isScalableLoopLegal(ElementCount VF,
+                                                     StringRef &Msg) const {
----------------
sdesmalen wrote:
> MaxVF ?
nit: s/isSclableLoopLegal/loopCanBeWidenedWithScalableVectors/ ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5648
+                                                     StringRef &Msg) const {
+  // Test that the loop-vectorizer can legalize all operations for this VF.
+  // FIXME: While for scalable vectors this is currently sufficient, this should
----------------
for eligible vectorization factors up to MaxVF


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5649
+  // Test that the loop-vectorizer can legalize all operations for this VF.
+  // FIXME: While for scalable vectors this is currently sufficient, this should
+  // be replaced by a more detailed mechanism that filters out specific VFs,
----------------
Can you move this comment down to the CallInst case?


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

https://reviews.llvm.org/D102394