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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks @aranisumedh for working on this!

The patch is quite big and I think it would be easier to review if it is split up. My main concern is that this patch conflates the SVFS pass/mechanism and the (demangling for the) AArch64 Vector ABI, which I think need to be split up into separate patches.

If you add a print method to the SVFS pass, you can print the available vector mappings as described in LLVM IR function attributes by running `opt -analyze -svfs`, which would make it easier to test using FileCheck. If you add that first, you could use the `-analyze` functionality to test the demangling of the AArch64 Vector ABI with `.ll` tests.

Not sure if this was already discussed anywhere else, but is there a reason this doesn't live in TargetLibraryInfo? There is already a `getVectorizedFunction` in that class, so it makes sense to make that method more powerful by having it take a `VectorFunctionShape` as well as `unsigned VF`.



================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:45
+  OMP_Uniform,       // declare simd uniform(i)
+  Unknown
+};
----------------
aranisumedh wrote:
> a.elovikov wrote:
> > How is this different from Vector?
> You mean `Unknown`? It's for error handling.
It is more explicit to rename it to Invalid in that case.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:81
+
+#define LOOKUP_PARAMETER(PARAM)                                                \
+  bool is##PARAM() { return ParamKind == ParameterKind::OMP_##PARAM; }
----------------
Can you just expand these functions instead of using a pragma? Given the simplicity of the definition there is little benefit to using a macro here, and having them expanded would  make it easier to search for in the codebase.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:100
+
+  static ParameterKind convertToParameterKind(StringRef kind) {
+    ParameterKind ParamKind = StringSwitch<ParameterKind>(kind)
----------------
is this not specific to the AArch64 Vector ABI?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:128
+  // for a llvm::CallInst
+  struct VectorFunctionShape {
+    unsigned VF;                          // Vectorization factor
----------------
VectorFunctionShape could benefit from having a constructor, if only to ensure that a VectorFunctionShape is valid and can be used as operand in `operator==`.


================
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;
+
----------------
Do we want to expose RecordTable?


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:179
+  Function *getVectorizedFunction(llvm::CallInst *Call,
+                                  VectorFunctionShape Info) const;
+};
----------------
This method asks for a vectorized function that *exactly*matches the specifications in VectorFunctionShape info.  You may want to think about a mechanism in VectorFunctionShape where you can distinguish "required"  and "optional" features. For example, if this function is asked for to return an unpredicated vector function, but it only has a predicated vector function, we'd still prefer to get the predicated vector function and construct an "all true" predicate, rather than having to fall back on scalar instructions.


================
Comment at: llvm/lib/Analysis/SearchVectorFunctionSystem.cpp:266
+  assert(Records.size() <= 2 && "Invalid Table Entries");
+  if (Records.size() == 2) {
+    StringRef VecName0 = VFABI::getVectorName(Records[0].VectorName);
----------------
I think you should either support 1 mapping, or support multiple mappings with a clear mechanism to prioritise which mapping takes priority. This function seems to prioritise based on whether or not a function-name is mangled which makes little sense to me.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64095