[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/include/llvm/Analysis/TargetTransformInfo.h:1523
   virtual bool isLegalMaskedExpandLoad(Type *DataType) = 0;
+  virtual bool isIntrinsicLegalForScalableVectors(Intrinsic::ID IID) = 0;
   virtual bool hasDivRemOp(Type *DataType, bool IsSigned) = 0;
----------------
nit: Ah I only now see it falls a bit out of style, so perhaps `isLegalIntrinsicForScalableVectors` is a better name. Sorry I didn't spot that before.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1744
+  switch (IID) {
+  case Intrinsic::abs: // Begin integer bit-manipulation.
+  case Intrinsic::bswap:
----------------
nit: maybe drop the comments, because it's not complete (for example, smax is not bit-manipulation)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1515
   /// variables found for the given VF.
-  bool canVectorizeReductions(ElementCount VF) {
+  bool canVectorizeReductions(ElementCount VF) const {
     return (all_of(Legal->getReductionVars(), [&](auto &Reduction) -> bool {
----------------
nit: is this an unrelated change?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5692-5699
+        for (auto &VFInfo : VFDatabase::getMappings(*CI)) {
+          if (VFInfo.Shape.IsScalable) {
+            HasScalableMapping = true;
+            break;
+          }
+        }
+
----------------
nit: Can you use `llvm::any_of()` instead of the above loop?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5706
+          return false;
+        }
+      }
----------------
Can you add a FIXME that says this code is still too optimistic? It depends on the chosen VF whether the loop can vectorize (vis-a-vis whether a mapping is available for that specific VF). I expect we'll want to build up a set of suitable VFs (up to and including MaxVF) that are legal, so that we can later filter out VFs that should not be considered as candidates for vectorization.


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

https://reviews.llvm.org/D102394