[PATCH] D96011: [NFC][Analysis] Change struct VecDesc to use ElementCount

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1612
 StringRef TargetLibraryInfoImpl::getVectorizedFunction(StringRef F,
-                                                       unsigned VF) const {
+                                                       ElementCount VF) const {
   F = sanitizeFunctionName(F);
----------------
nit: `const ElementCount &`


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:118
+  // TODO: Add scalable variants once we're able to test them.
+  assert(WidestScalableVF.isZero() && "Scalable vector mappings unsupported");
 
----------------
nit: s/unsupported/not yet supported/


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:598
+  if (Scalarize) {
+    ElementCount WidestFixedVF = ElementCount::getNull();
+    ElementCount WidestScalableVF = ElementCount::getNull();
----------------
Should we just add a constructor that takes no arguments and does the same as getNull?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:604
       Scalarize &= !TLI.isFunctionVectorizable(ScalarName, VF);
-    }
+    for (ElementCount VF = ElementCount::getScalable(2);
+         ElementCount::isKnownLE(VF, WidestScalableVF); VF *= 2)
----------------
This should be `Element::getScalable(1)` because `<vscale x 1 x <eltty>>` may well be a valid type for some scalable vector architecturs. For SVE we don't loops to be vectorized with this VF because it is expensive to legalize, but we shouldn't work with that assumption for generic code.


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

https://reviews.llvm.org/D96011