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

[Sumedh Arani via Phabricator via llvm-commits]

aranisumedh marked 15 inline comments as done.
aranisumedh added inline comments.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:120
+
+struct SVFS {
+  Module *M;
----------------
a.elovikov wrote:
> jdoerfert wrote:
> > Doxygen class comment.
> This is marked as done but I don't see the doxygen comment...
That is because it was a part of the last diff. Hence, the lines get moved. The getter and setters are pretty straightforward. Do I still add a doxygen comment for the same?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:96
+
+  void setLinearStepOrPos(int s) { LinearStepOrPos = s; }
+
----------------
aranisumedh wrote:
> 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.
The current implementation allows me to handle corner cases better. This setter function is used to set the `LinearStepOrPos` for the first time. 



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:100
+
+  static ParameterKind convertToParameterKind(StringRef kind) {
+    ParameterKind ParamKind = StringSwitch<ParameterKind>(kind)
----------------
sdesmalen wrote:
> is this not specific to the AArch64 Vector ABI?
No. IIRC, this is in agreement with other vendors too. 


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:153
+  // Uses a vector as this table will not have a large entries
+  SmallVector<VectorRecord, 8> RecordTable;
+
----------------
sdesmalen wrote:
> Do we want to expose RecordTable?
Making it private.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64095