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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some high level coding comments.

Documentation needed in various places.

Why `std::vector` and not `llvm::SmallVector` for example?



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:35
+    OMP_Uniform
+};
+
----------------
I'd like some "class comment" and also comments on the members. It is not clear to everyone (incl. me) what distinguishes for example `LinearValPos` and `LinearUValPos` and why we need that.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:48
+class ParamType {
+    private:
+        unsigned ParamPos;
----------------
automatically private.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:54
+    public:
+        // Constructor - Used for testing
+        ParamType(unsigned ParamPos, StringRef ParamKind, int LinearStepOrPos) {
----------------
I don't think this is a good idea.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:62
+        // Constructor for ParameterKind with no step. Eg- ParameterKind::Vector
+        ParamType(unsigned ParamPos, StringRef ParamKind) {
+            this->ParamPos = ParamPos;
----------------
`) : ParamPos(ParamPos), LinearStepOrPos(0) `

Why take a string just to convert it? Shouldn't we expose the conversion as a static member and expect a enum here. That way users can use the conversion if they need to but if they know the enums they want there is no need to go through the string first.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:112
+            return ParamKind == ParameterKind::OMP_Uniform;
+        }
+
----------------
One could generate them with a macro if one wanted to:

```
#declare LOOKUP_FN(NAME) \
   bool is ##NAME () { \
            return ParamKind == ParameterKind::OMP_ ## NAME; \
   }

LOOKUP_FN(Uniform)
...
#undef LOOKUP_FN(NAME)
```


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:137
+                .Case("Us", ParameterKind::OMP_LinearUValPos)
+                .Case("u",  ParameterKind::OMP_Uniform);
+        }
----------------
The default case is missing. `llvm_unreachable` I assume.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:150
+class SVFS{
+    public:
+        Module *M;
----------------
Make it a struct or do not expose all members.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:153
+
+        SVFS(Module *M) {
+            this->M = M;
----------------
`) : M(M) {}`


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:157
+
+        typedef struct VectorFunctionShape {
+            unsigned VF; // Vectorization factor 
----------------
No need for the typedef. Again, please add comments to the class and members.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:163
+            std::vector<ParamType> Parameters;
+            bool operator==(const VectorFunctionShape &other) const {
+                return (VF == other.VF
----------------
(Honest question) Can't we use something like `= default` or is that a C++ feature we are not allowed to use yet?


================
Comment at: llvm/lib/Analysis/SearchVectorFunctionSystem.cpp:20
+    ABISignature = MangledName.substr(0,4);
+    return ABISignature.equals("_ZGV");
+}
----------------
`MangledName.startswith("_ZGV")` ?


================
Comment at: llvm/lib/Analysis/SearchVectorFunctionSystem.cpp:36
+			.Case("d", ISAKind::ISA_AVX2)
+			.Case("e", ISAKind::ISA_AVX512);
+	return ISA;
----------------
You could add an "ISA_UNKNOWN" case as a default here and assert on that one. Would probably make it slightly easier to maintain. But this is fine as well.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64095