[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:54
+    public:
+        // Constructor - Used for testing
+        ParamType(unsigned ParamPos, StringRef ParamKind, int LinearStepOrPos) {
----------------
jdoerfert wrote:
> I don't think this is a good idea.
I forgot to remove the comment. This is incorrect comment. 

This constructor is not used for testing.


================
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;
----------------
jdoerfert wrote:
> `) : 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.
I preferred to use a string as demangling does String parsing. Hence, I thought it would be easier to call a class constructor with the StringRef as it's argument.



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:137
+                .Case("Us", ParameterKind::OMP_LinearUValPos)
+                .Case("u",  ParameterKind::OMP_Uniform);
+        }
----------------
jdoerfert wrote:
> The default case is missing. `llvm_unreachable` I assume.
I don't think StringSwitch Default Case handles `llvm_unreachable`



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:163
+            std::vector<ParamType> Parameters;
+            bool operator==(const VectorFunctionShape &other) const {
+                return (VF == other.VF
----------------
jdoerfert wrote:
> (Honest question) Can't we use something like `= default` or is that a C++ feature we are not allowed to use yet?
It is a C++ 20 feature. Hence, I think I cannot use it for now. https://en.cppreference.com/w/cpp/keyword/default


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

https://reviews.llvm.org/D64095