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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 7 inline comments as done.
fpetrogalli added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:124
+void setVectorVariantNames(CallInst *CI,
+                           const SmallSet<std::string, 8> &VariantMappings);
+
----------------
sdesmalen wrote:
> Can these use StringRef?
Indeed they can. Thank you.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:1184
+  }
+}
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:317
+  CI->setAttributes(Attrs);
+}
----------------
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...


================
Comment at: llvm/unittests/Analysis/VectorFunctionABITest.cpp:482
+  EXPECT_EQ(Mappings, Exp);
+}
----------------
jdoerfert wrote:
> A `static LLVMContext` seems risky if the test run in parallel (idk if they do but they could maybe).
> Other unit tests have a setup and teardown method and own their module instead of using the constructor.
> And now I see there is also a Ctx member, I'm confused.
Bad coding, sorry. I fixed both tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69976