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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:97
+  // argument via \p HasGlobalPred.
+  static VFShape getAllVectorsParams(const CallInst &CI, const unsigned VF,
+                                     const bool IsScalable,
----------------
fpetrogalli wrote:
> I have extracted the VFShape API here: https://reviews.llvm.org/D70513 (it is a work in progress as of now, I might finish up things later today).
> 
> 
I think this can take `FunctionType` instead of `CallInst`.

> I have renamed to VFShape::getAllVectorsParams
nit: I know this is bikeshedding, but what do you think of `VFShape::widenAllParams`?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:98
+  static VFShape getAllVectorsParams(const CallInst &CI, const unsigned VF,
+                                     const bool IsScalable,
+                                     const bool HasGlobalPred) {
----------------
these parameters shouldn't use `const`.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:98
+  static VFShape getAllVectorsParams(const CallInst &CI, const unsigned VF,
+                                     const bool IsScalable,
+                                     const bool HasGlobalPred) {
----------------
sdesmalen wrote:
> these parameters shouldn't use `const`.
Is there a reason to pass VF and IsScalable separately, instead of passing an `ElementCount` ?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:197
+      if (Shape.hasValue() && (Shape.getValue().ScalarName == ScalarName)) {
+        assert(CI.getModule()->getFunction(Shape.getValue().VectorName));
+        Mappings.push_back(Shape.getValue());
----------------
Please add a message to the assert.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:205
+  /// Retrieve all the VFInfo instances associated to the CallInst CI.
+  static SmallVector<VFInfo, 8> getMappings(const CallInst &CI) {
+    SmallVector<VFInfo, 8> Ret;
----------------
this method doesn't do anything more than invoke `getVFABIMappings` so has no value (other than being public).


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:103
+    }
+    if (HasGlobalPred)
+      Parameters.push_back(
----------------
fpetrogalli wrote:
> sdesmalen wrote:
> > is `HasGlobalPred` the same as `isPredicated`?
> > 
> > Also, is the predicate always known/expected to be the last parameter by the Vector ABI?
> > is HasGlobalPred the same as isPredicated?
> 
> Th GlobalPredicate is listed in the `VFParamKind` enum class:
> 
> ```
>   GlobalPredicate,   // Global logical predicate that acts on all lanes
>                      // of the input and output mask concurrently. For
> 		     // example, it is implied by the `M` token in the
> 		     // Vector Function ABI mangled name.
> ```
> 
> It is a special predicate because it differ from the parameter masks that can be individually attached to the parameters. These kind of masks are not handled yet by the VFParamKind, but I understand that they were needed by @simoll when discussing the RFC.
> 
> > Also, is the predicate always known/expected to be the last parameter by the Vector ABI?
> 
> For all the Vector Function ABIs supported at the moment in LLVM, yes. If vendor X decides to produce an ABI were the mask is the first parameter, we will have to change this code. But for now, we will assume the global predicate is the last parameter.
> It is a special predicate because it differ from the parameter masks that can be individually attached to the parameters.
I'd expect the operation to be predicated, not the individual operands. What would be the meaning of a predicated operand?

> For all the Vector Function ABIs supported at the moment in LLVM, yes. If vendor X decides to produce an ABI were the mask is the first parameter, we will have to change this code.
Can we add an assert somewhere to enforce the assumption that the global predicate is passed as the last operand?


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:217
+  Function *getVectorizedFunction(const VFShape &Shape) const {
+    for (const auto &Info : ScalarToVectorMappings)
+      if (Info.Shape == Shape)
----------------
fpetrogalli wrote:
> sdesmalen wrote:
> > 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`.
> sort them by what field? VF? 
> 
> Also, I don't expect an attribute to hold more than 8 functions , which seems to be the worst case scenario when all X86 vector extensions are being used... are you sure you want to add such optimization.
> 
> How about we leave it as it is (slow but simple) and leave optimizations for the future if it turns out we need to speed up the search?
Alright, if it only has a few elements and is constructed to do a single lookup it is probably not worth the overhead of sorting.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1848
         if (Call && !Call->isNoBuiltin() && Call->getCalledFunction() &&
-            TLI->isFunctionVectorizable(Call->getCalledFunction()->getName()))
+            !VFDatabase::getMappings(*Call).empty())
           continue;
----------------
fpetrogalli wrote:
> sdesmalen wrote:
> > `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.
> Done, check the implementation of `getVFABIMappings`.
The early exit in getMappings doesn't stop a SmallVector from having to be created/destroyed. It would be better to create a new method such as `bool hasVectorVariants()` that answers the question directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67572