[PATCH] D69976: [VFABI] Read/Write functions for the VFABI attribute.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:138
+void getVectorVariantNames(const CallInst &CI,
+                           SmallVector<std::string, 8> &VariantMappings);
 } // end namespace VFABI
----------------
It would be better passed as an `ArrayRef<std::string>`, or otherwise a `SmallVectorImpl<std::string> &VariantMappings`.


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:124
+void setVectorVariantNames(CallInst *CI,
+                           const SmallSet<std::string, 8> &VariantMappings);
+
----------------
jdoerfert wrote:
> fpetrogalli wrote:
> > fpetrogalli wrote:
> > > sdesmalen wrote:
> > > > Can these use StringRef?
> > > Indeed they can. Thank you.
> > It turned out that the assertion on invalid vector ABI names fire when running the code on the TLI tests in the loop vectorizer. It looks like the underlying memory of the StringRefs is modified. I have added some debugging and it shows that the mappings values are modified somewhere in the pipeline. I'll revert back to std::string for now.
> > 
> > FWIW, StringRef is not supposed to store data (see doxygen docs), but just to operate on buffers that owned by other component. This means that StringRef cannot be used as a return parameter of a function, because the buffer set up in the function is destroyed when leaving the function. We are not using StringRef as a return value here, but I guess that we  incur in a similar problem by using StringRef in a reference parameter that is in fact an output value.
> FWIW, It will not work here because the "strings" you put in there do not outlive the function on their own. One can return strings in a vector of StringRefs in different situations.
Great, thanks for clarifying!

I would however recommend using `ArrayRef` or `SmallVectorImpl`, instead of `SmallVector<std::string, 8>`.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1172
+#ifndef NDEBUG
+  const Module *M = CI.getModule();
+#endif
----------------
The code would be simpler to read if we let the compiler hoist `M` out of the loop below, so please move it to the second `NDEBUG` block.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:304
+  Out << "_ZGV" << _LLVM_TLI_ << "N" << VF;
+  for (unsigned I = 0; I < CI->getNumArgOperands(); ++I) {
+    Out << "v";
----------------
nit: unnecessary curly braces.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:327
+                                  const StringRef VFName) {
+  assert(CI && "Invalid CallInst.");
+  Module *M = CI->getParent()->getParent()->getParent();
----------------
nit: is this assert really necessary?


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:353
+void VFABI::addMappingsFromTLI(const TargetLibraryInfo *TLI, CallInst *CI) {
+  assert(TLI && "Invalid TLI.");
+  assert(CI && "Invalid CallInst.");
----------------
are these asserts necessary?


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:365
+  const std::string ScalarName = CI->getCalledFunction()->getName();
+  // Nothing to be done if the TLI things the function is not
+  // vectorizable.
----------------
nit: the comment doesn't really clarify more than the code already expresses, so can be removed.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:376
+    const std::string TLIName = TLI->getVectorizedFunction(ScalarName, VF);
+    if (TLIName != "") {
+      std::string MangledName = mangleTLIName(TLIName, CI, VF);
----------------
`if (!TLIName.empty())`


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:378
+      std::string MangledName = mangleTLIName(TLIName, CI, VF);
+      //      List.push_back(MangledName);
+      SetOfMangledNames.insert(MangledName);
----------------
remove?


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:391
+    CallInst *CI, const SmallVector<std::string, 8> &VariantMappings) {
+  assert(CI && "Invalid CallInst");
+
----------------
is this assert necessary?


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

https://reviews.llvm.org/D69976