[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.

Second round, we make progress :)

Style:
We usually prefer C++-style comments `//` if possible. They are placed in front of variables (not for enums). And you should use doxygen style comments whenever it'll get picked up, so in front of any declaration (class, member, ...). (see also the style stuff here: https://llvm.org/docs/CodingStandards.html)

Test:
It looks like the tests are all positive, e.g., good input results in good return. Could you add some to make sure invalid input is detected _and_ reported as such?



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:91
+  LOOKUP_PARAMETER(LinearUValPos);
+  LOOKUP_PARAMETER(Uniform);
+
----------------
Why not `LOOKUP_PARAMETER(Vector)`?

I don't think you want `;` at the end.

Cleanup:
`#undef LOOKUP_PARAMETER`


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:110
+                                  .Case("Us", ParameterKind::OMP_LinearUValPos)
+                                  .Case("u", ParameterKind::OMP_Uniform);
+
----------------
What is the default here? What happens for bad inputs?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:120
+
+struct SVFS {
+  Module *M;
----------------
Doxygen class comment.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:121
+struct SVFS {
+  Module *M;
+
----------------
I found a single use of this exposed member and that use can be replaced by something like:
`Call->getModule()`
If there is no reason to have this member, get rid of it and all the `init` stuff.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:161
+  /// the vector function signature
+  void createTableLookupRecord(CallInst *Call);
+
----------------
These doxygen documentations do not match the methods.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:196
+bool getIsScalable(StringRef MangledName);
+} // end namespace VFABI
+
----------------
These methods lack doxygen documentation.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:216
+  }
+};
+
----------------
It seems the new pass manager interface is missing.


================
Comment at: llvm/lib/Analysis/SearchVectorFunctionSystem.cpp:277
+      for (auto &I : B)
+        if (auto *Call = dyn_cast<CallInst>(&I))
+          S.createTableLookupRecord(Call);
----------------
`for (Instruction &I : instructions(F))`


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

https://reviews.llvm.org/D64095