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

[Andrei Elovikov via Phabricator via llvm-commits]

a.elovikov added a comment.

Thanks for working on this! I'm really interested in using this functionality so here are some comments on the main interface header.



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:120
+
+struct SVFS {
+  Module *M;
----------------
jdoerfert wrote:
> Doxygen class comment.
This is marked as done but I don't see the doxygen comment...


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:45
+  OMP_Uniform,       // declare simd uniform(i)
+  Unknown
+};
----------------
How is this different from Vector?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:56
+  ISA_AVX512,
+  ISA_Unknown
+};
----------------
That's great, I see how it can be used for non-OMP usage.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:61
+class ParamType {
+  unsigned ParamPos;       // Parameter Position
+  ParameterKind ParamKind; // Kind of Parameter
----------------
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?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:66
+public:
+  ParamType(unsigned ParamPos, ParameterKind ParamKind, int LinearStepOrPos)
+      : ParamPos(ParamPos), ParamKind(ParamKind),
----------------
My preference for this is to make the ctor private and provide public static functions to create different set of required arguments for different kinds. E.g. no LinearStepOrPos of varying/vector parameter. SOmething like this:


```
private:
  ParamType()

public
  static ParamType vector() { return ParamType{}; }
  static ParamType linear(int LinearStep) { ... }
  static ParamType linearWithNonConstStride(unsigned StepArgPosition) { ... }
```

Feel free to ignore though.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:84
+
+  LOOKUP_PARAMETER(Linear)
+  LOOKUP_PARAMETER(LinearVal)
----------------
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.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:94
+
+  int getLinearStepOrPos() { return LinearStepOrPos; }
+
----------------
I would split it into two methods with proper asserts for the correct kind of this param.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:96
+
+  void setLinearStepOrPos(int s) { LinearStepOrPos = s; }
+
----------------
Do we really need the setter? Why once created param description could change?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:131
+    bool IsMasked;                        // True if Masked
+    bool IsScalable;                      // True if Scalable
+    ISAKind ISA;                          // Instruction Set Arch
----------------
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?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:134
+    SmallVector<ParamType, 8> Parameters; // List of Parameter Info
+
+    // Equality checks
----------------
Is it possible to add the information about the return value? Like varying vector/uniform scalar/uniform vector.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:171
+  SmallVector<VectorFunctionShape, 8>
+  isFunctionVectorizable(CallInst *Call) const;
+
----------------
I'd rather have an interface not referencing the CallInst (e.g. mangled name instead). Why doesn't it have a "bool Masked" parameter?


================
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;
----------------
Same as above, I think. E.g. interface without CallInst, bool Mask.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:195
+/// \param Result True if Masked
+bool getIsMasked(StringRef MangledName);
+/// Gets vectorization factor
----------------
I'm not sure why is in VFABI namespace. Mask/NoMask isn't ABI-specific. Same for functions below.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64095