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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 16 inline comments as done.
fpetrogalli 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,
----------------
sdesmalen wrote:
> 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`?
> I think this can take FunctionType instead of CallInst

Yes, but... what's the point? We will have to introduce one or two extra calls to get the FunctionType... All we need is the number of arguments of the function, so in fact the function could just take an unsigned integer. But I don't like it. The fact that I use  CallInst is cleaner and not worse than having an optimized interface. But this is my personal preference, so if you ask nicely  :P I will change the interface.

> nit: I know this is bike shedding, but what do you think of VFShape::widenAllParams?

I know where you come from, the Vectorizer :). I'd rather not, I really want to have a static `get` method attached to the VFShape. It seems to be in the style of LLVM to use `get` for static public member functions that build objects (see http://llvm.org/doxygen/classllvm_1_1VectorType.html). Since this is the only `get` method of `VFShape`, I have renamed it to `get`.

For the record, the changes have been applied to https://reviews.llvm.org/D70513

This function this disappear from this revision. 


================
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:
> sdesmalen wrote:
> > 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`?
> > I think this can take FunctionType instead of CallInst
> 
> Yes, but... what's the point? We will have to introduce one or two extra calls to get the FunctionType... All we need is the number of arguments of the function, so in fact the function could just take an unsigned integer. But I don't like it. The fact that I use  CallInst is cleaner and not worse than having an optimized interface. But this is my personal preference, so if you ask nicely  :P I will change the interface.
> 
> > nit: I know this is bike shedding, but what do you think of VFShape::widenAllParams?
> 
> I know where you come from, the Vectorizer :). I'd rather not, I really want to have a static `get` method attached to the VFShape. It seems to be in the style of LLVM to use `get` for static public member functions that build objects (see http://llvm.org/doxygen/classllvm_1_1VectorType.html). Since this is the only `get` method of `VFShape`, I have renamed it to `get`.
> 
> For the record, the changes have been applied to https://reviews.llvm.org/D70513
> 
> This function this disappear from this revision. 
Please take any other further review of `getAllVectorParams` to https://reviews.llvm.org/D70513


================
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:
> sdesmalen wrote:
> > these parameters shouldn't use `const`.
> Is there a reason to pass VF and IsScalable separately, instead of passing an `ElementCount` ?
> these parameters shouldn't use const.

I disagree, especially for the CI argument. The method is not supposed to change the reference.

I have anyway removed the const from the other parameters.

(again, in https://reviews.llvm.org/D70513, not here, but I will update this patch)


================
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;
----------------
sdesmalen wrote:
> this method doesn't do anything more than invoke `getVFABIMappings` so has no value (other than being public).
> this method doesn't do anything more than invoke getVFABIMappings so has no value (other than being public).

Yeah, but the value is int he comment inside of it, stating that other VFShapes can be build outside of a VFABI context. I'd prefer to keep it.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:103
+    }
+    if (HasGlobalPred)
+      Parameters.push_back(
----------------
sdesmalen wrote:
> 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?
> I'd expect the operation to be predicated, not the individual operands. What would be the meaning of a predicated operand?

This was a requirement from @simoll, he needs to have masking associated to each of the operands.

> Can we add an assert somewhere to enforce the assumption that the global predicate is passed as the last operand?

Is is done in https://reviews.llvm.org/D70513


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1848
         if (Call && !Call->isNoBuiltin() && Call->getCalledFunction() &&
-            TLI->isFunctionVectorizable(Call->getCalledFunction()->getName()))
+            !VFDatabase::getMappings(*Call).empty())
           continue;
----------------
sdesmalen wrote:
> 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.
I understand your concerns in performance here, but I am not keen in doing this. The attribute might contain junk that don't demangle correctly to a VFInfo - in that case the function would return "yes, this call has vector variant", but that wouldn't make sense because there would be no variant. I'd rather play it safe.


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

https://reviews.llvm.org/D67572