[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/include/llvm/Analysis/VectorUtils.h:194
 /// <vectorname> = the name of the vector function.
 std::string mangleTLIVectorName(StringRef VectorName, StringRef ScalarName,
+                                unsigned numArgs, ElementCount VF);
----------------
Can you pull this change out into a separate patch? Then the remainder of this patch becomes NFC (especially when you add asserts the VF must be fixed-width for the entries).


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1671-1672
+  // same as a scalar.
+  ScalableVF = 0;
+  FixedVF = 1;
   if (ScalarF.empty())
----------------
Can you define these as ElementCount (and change the interface to take an ElementCount instead of unsigned), and do the initialization before the call to getWidestVF?


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1679
   while (I != VectorDescs.end() && StringRef(I->ScalarFnName) == ScalarF) {
-    if (I->VectorizationFactor > VF)
-      VF = I->VectorizationFactor;
+    ElementCount VF = I->VectorizationFactor;
+    if (VF.isScalable() && VF.getKnownMinValue() > ScalableVF)
----------------
nit:
  ElementCount *Ptr = VF.isScalable() ? &ScalableVF : &FixedVF;
  if (ElementCount::isKnownGT(VF, *Ptr))
    *Ptr = VF;


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:112
+
+  for (unsigned VF = 2; VF <= WidestFixedVF; VF *= 2) {
+    AddVariantDecl(ElementCount::getFixed(VF));
----------------
nit: unnecessary curly braces.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:116
 
+  // TODO: Add scalable variants once we're able to test them.
+
----------------
This needs an assert to make sure WidestScalableVF is not set, so that it is not silently ignoring any scalable entries.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:604-607
+    for (unsigned VF = 2; VF <= WidestScalableVF; VF *= 2) {
+      Scalarize &= !TLI.isFunctionVectorizable(ScalarName,
+                                               ElementCount::getScalable(VF));
+    }
----------------
This can probably return false directly when WidestScalableVF is > 0, because there is no way to scalarize for scalable VFs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96011