[PATCH] D67572: [SVFS] The Search Vector Function System.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:201
+
+  void fixUpVFABINames(CallInst *CI) const;
 };
----------------
Documentation missing, and please do not call it "fixup", maybe "adjust" or something but not "fixup".


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:266
     return Impl->getVectorizedFunction(F, VF);
   }
 
----------------
Why do you privatize these redirects? I would expect them to be used, thus public, or unused, thus deleted.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:58
+#define _LLVM_INTERNAL_TLI_ "_LLVM_INTERNAL_TLI_"
+
 /// Encapsulates information needed to describe a parameter.
----------------
I generally dislike non-scoped macros. Is there a reason a `static constexpr char*` doesn't work?
I would also opt for a shorter name, "_llvm_" or "_llvm_tli_" but that is debatable.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:132
 /// _ZGV<isa><mask><vlen><parameters>_<scalarname>[(<redirection>)].
-Optional<VFInfo> tryDemangleForVFABI(StringRef MangledName);
+Optional<VFInfo> tryDemangleForVFABI(const std::string Input);
 
----------------
I doubt this change is necessary. Only if you need to own the underlying data you need to replace StringRef with std::string.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:142
+void setVectorVariantNames(CallInst *CI,
+                           const SmallSet<std::string, 8> VariantMappings);
+
----------------
I'd prefer
 `void getVectorVariantNames(CallInst *CI, SmallSet<std::string, 8> &VariantMappings);`
but for sure you want a reference here:
```
void setVectorVariantNames(CallInst *CI,
                           const SmallSet<std::string, 8> &VariantMappings);
```


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

https://reviews.llvm.org/D67572