[PATCH] D67572: [VectorUtils] Introduce the Vector Function Database (VFDatabase).

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks @fpetrogalli for updating this patch!

> Down to two failures.
>  Failing Tests (2):

Just checking, are these failures resolved in the latest revision?



================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:92
+  }
+  // Retrive a flat vectorization shape of the function, where all
+  // parameters are mapped to vector with \p VF lanes. Specifies
----------------
nit: retrive -> retrieve


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:97
+  // HasGlobalPred.
+  static VFShape getFlatVF(const CallInst &CI, const unsigned VF,
+                           const bool IsScalable, const bool HasGlobalPred) {
----------------
I don't really understand what you mean by a "flat" vectorization shape. Is this function supposed to return a 'widened' (vector version of a) function? If so, can we please rename this to `getVectorShapeForCall` ?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:100
+    SmallVector<VFParameter, 8> Parameters;
+    for (unsigned I = 0; I < CI.arg_size(); ++I) {
+      Parameters.push_back(VFParameter({I, VFParamKind::Vector}));
----------------
nit: unnecessary curly braces


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:103
+    }
+    if (HasGlobalPred)
+      Parameters.push_back(
----------------
is `HasGlobalPred` the same as `isPredicated`?

Also, is the predicate always known/expected to be the last parameter by the Vector ABI?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:169
+class VFDatabase {
+private:
+  const CallInst &CI; /// The CallInst for which we are looking for vector
----------------
nit: in a class members are private by default.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:170
+private:
+  const CallInst &CI; /// The CallInst for which we are looking for vector
+                      /// functions.
----------------
nit: put comments above variable


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:179
+                               SmallVectorImpl<VFInfo> &Mappings) {
+    const std::string ScalarName = CI.getCalledFunction()->getName();
+    SmallVector<std::string, 8> ListOfStrings;
----------------
StringRef ?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:188
+      // function in the Module M.
+      if (Shape.hasValue())
+        if ((Shape.getValue().ScalarName == ScalarName) &&
----------------
nit: These two if-statements can be merged with a `&&`


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:190
+        if ((Shape.getValue().ScalarName == ScalarName) &&
+            CI.getModule()->getFunction(Shape.getValue().VectorName))
+          Mappings.push_back(Shape.getValue());
----------------
should `CI.getModule()->getFunction(Shape.getValue().VectorName)` be an assert? When would it ever happen that the vector function is not //declared// * in the IR?

*note that I'm specifically saying //declared// and not //defined// here.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:217
+  Function *getVectorizedFunction(const VFShape &Shape) const {
+    for (const auto &Info : ScalarToVectorMappings)
+      if (Info.Shape == Shape)
----------------
Is it worth implementing a `operator<` for VFShape and sorting the result for `getMappings()`?
That way we can use binary search using llvm::lower_bound instead of looping through each shape in `ScalarToVectorMappings`.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1848
         if (Call && !Call->isNoBuiltin() && Call->getCalledFunction() &&
-            TLI->isFunctionVectorizable(Call->getCalledFunction()->getName()))
+            !VFDatabase::getMappings(*Call).empty())
           continue;
----------------
`getMappings` is quite an expensive call, so you'll want to add a special function here that bails out earlier, and doesn't have to demangle string and populate (and possibly sort) an array.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3254
+  if (!TLI ||
+      !VFDatabase(*CI).isFunctionVectorizable(VFShape::getFlatVF(
+          *CI, VF, false /*IsScalable*/, false /*HasGlobalPred*/)) ||
----------------
the call to `isFunctionVectorizable` is expensive, so please reorder this after `CI->isNoBuiltin()`, so that the function can bail out more cheaply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67572