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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
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) {
----------------
fpetrogalli wrote:
> sdesmalen wrote:
> > 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` ?
> Yeah, the name is misleading. This is supposed to return a widenened version of the function... but I don't like the name `getVectorShapeForCall`, because a Shape for a Vector Call can have Linear modifiers for example, while this function is returning only the shape that uses `VFParamKind::Vector` for all `VFPArameter`s. 
> 
> How about `getAllVectorsShape`?
I explained the meaning of the function in the comment.


================
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) {
----------------
fpetrogalli wrote:
> fpetrogalli wrote:
> > sdesmalen wrote:
> > > 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` ?
> > Yeah, the name is misleading. This is supposed to return a widenened version of the function... but I don't like the name `getVectorShapeForCall`, because a Shape for a Vector Call can have Linear modifiers for example, while this function is returning only the shape that uses `VFParamKind::Vector` for all `VFPArameter`s. 
> > 
> > How about `getAllVectorsShape`?
> I explained the meaning of the function in the comment.
I have renamed to `VFShape::getAllVectorsParams`.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:103
+    }
+    if (HasGlobalPred)
+      Parameters.push_back(
----------------
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.


================
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());
----------------
sdesmalen wrote:
> 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.
> 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?

Yes, done. In fact, and IR where there is a VFABI attribute with a mapping to function X, but doesn't have X declared, shoudl be considered broken.

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

You are reaching enlightenment! :)


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:217
+  Function *getVectorizedFunction(const VFShape &Shape) const {
+    for (const auto &Info : ScalarToVectorMappings)
+      if (Info.Shape == Shape)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67572