[PATCH] D64095: SVFS implementation according to RFC: Interface user provided vector functions with the vectorizer.

[Sumedh Arani via Phabricator via llvm-commits]

aranisumedh added inline comments.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:45
+  OMP_Uniform,       // declare simd uniform(i)
+  Unknown
+};
----------------
a.elovikov wrote:
> How is this different from Vector?
You mean `Unknown`? It's for error handling.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:61
+class ParamType {
+  unsigned ParamPos;       // Parameter Position
+  ParameterKind ParamKind; // Kind of Parameter
----------------
a.elovikov wrote:
> Is it original (scalar) call position or vector call position? Please make it clear in the comment.
> 
> I'm interested in this because my use case might have Mask argument in the vector function variant in the middle of other arguments. Is it possible to handle that with this patch?
It is the original call position. 


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:84
+
+  LOOKUP_PARAMETER(Linear)
+  LOOKUP_PARAMETER(LinearVal)
----------------
a.elovikov wrote:
> I'm not sure if more broad "isLinear" interface for all the Linear* would be useful. Kind of sad if we won't be able to have it because of this accessors.
It just checks if the parameter type is OMP_Linear. It doesn't check for `Linear*` case


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:94
+
+  int getLinearStepOrPos() { return LinearStepOrPos; }
+
----------------
a.elovikov wrote:
> I would split it into two methods with proper asserts for the correct kind of this param.
According to the VFABI, it'll be either step or pos associated. If we split this into two, we would need more variables. 

As per the ABI, if it's `ls, Rs, Ls, Us, u`, only then would we pos associated with the parameter else it's a step.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:96
+
+  void setLinearStepOrPos(int s) { LinearStepOrPos = s; }
+
----------------
a.elovikov wrote:
> Do we really need the setter? Why once created param description could change?
No. I don't think the parameters can change. I can change the implementation so as to avoid using the setter function.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:131
+    bool IsMasked;                        // True if Masked
+    bool IsScalable;                      // True if Scalable
+    ISAKind ISA;                          // Instruction Set Arch
----------------
a.elovikov wrote:
> Isn't it part of ISA somehow? Why do we need a separate flag that isn't applicable for all the ISAs? Or do I miss something here?
> 
> Also, I'm not familiar with is (I assume it's related to SVE) - how does it deal with VF. Do we still need VF here if IsScalable is true?
If the VF is set to 'x' and and the ISA is SVE, it is then the IsScalable flag is set to true.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:134
+    SmallVector<ParamType, 8> Parameters; // List of Parameter Info
+
+    // Equality checks
----------------
a.elovikov wrote:
> Is it possible to add the information about the return value? Like varying vector/uniform scalar/uniform vector.
Could you elaborate, so as to make sure I get you right?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:171
+  SmallVector<VectorFunctionShape, 8>
+  isFunctionVectorizable(CallInst *Call) const;
+
----------------
a.elovikov wrote:
> I'd rather have an interface not referencing the CallInst (e.g. mangled name instead). Why doesn't it have a "bool Masked" parameter?
We are trying to search for all the vector function mappings available for a given scalar function by querying the SVFS table. Hence, I think that it is better to pass the CallInst, where we check for records in our table with matching scalar names. It also follows the description in the RFC.




================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:178
+  /// \param Result Available vector function shapes for vectorization
+  Function *getVectorizedFunction(llvm::CallInst *Call,
+                                  VectorFunctionShape Info) const;
----------------
a.elovikov wrote:
> Same as above, I think. E.g. interface without CallInst, bool Mask.
You can specify the bool Mask within the VectorFunctionShape that is the second parameter. In the separate patch that is to follow, I'll demonstrate how the loop vectorizer will call and query SVFS to vectorize a CallInst.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:195
+/// \param Result True if Masked
+bool getIsMasked(StringRef MangledName);
+/// Gets vectorization factor
----------------
a.elovikov wrote:
> I'm not sure why is in VFABI namespace. Mask/NoMask isn't ABI-specific. Same for functions below.
The parsing of the mangled names is as described in the Vector Function ABI. All these functions take in mangled name and parse the information. Hence, I decided to keep these function within the VFABI namespace.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64095