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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I have only minor comments left (below), @sdesmalen you wanna take a look and accept?



================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:124
+void setVectorVariantNames(CallInst *CI,
+                           const SmallSet<std::string, 8> &VariantMappings);
+
----------------
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.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1184
+  }
+}
----------------
fpetrogalli wrote:
> jdoerfert wrote:
> > To get the attribute:
> >   `CI->getAttribute(AttributeList::FunctionIndex, "vector-function-abi-variant")`
> > To get the module:
> >   `CI->getModule()`
> > 
> > Is the container choice (set) on purpose? Do we need to worry about non-determinism (I usually do!)? Do we use it as a set or would a vector do as well?
> > Is the container choice (set) on purpose? Do we need to worry about non-determinism (I usually do!)? Do we use it as a set or would a vector do as well?
> 
> Could do with a vector as well. It's just that I didn't want to take care of the accidental possibility of seeing duplicates in the IR. Not a functional problem itself, just something without any particular meaning.
If you need deduplication -> `SetVector` :)


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:317
+  CI->setAttributes(Attrs);
+}
----------------
fpetrogalli wrote:
> jdoerfert wrote:
> > I would have made it something like this, though it is mostly style:
> > ```
> > if (VariantMappings.empty())
> >   return;
> > for (const std::string &VariantMapping : VariantMappings)
> >   Out << VariantMapping << ",";
> > Buffer.pop_back();
> > ... Buffer.str(); 
> > ```
> > 
> > In the NDEBUG:
> > `for (const std::string &VariantMapping : VariantMappings)`
> > 
> > We need a variable to hold this string "vector-...".
> > 
> > Attribute addition:
> > `CI->addAttribute(AttributeList::FunctionIndex, Attribute::get(C, MISSING_VAR_FOR_STR, Buffer.out())`
> > 
> > We need a variable to hold this string "vector-...".
> 
> Sure. Where? If I add it in one header file, it complains of not being used in other places where the header file is not used. I have added it in the compilation unit, but in there it replaces only a single use...
See above :)


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:286
+// Name of the attribute where the variant mappings are stored.
+static const char *MappingsAttrName = "vector-function-abi-variant";
+
----------------
Now this is better but having it defined multiple times is bad.
Single location (`static constexpr char*`) should work or single location (`extern const char *`) with a single definition somewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69976