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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

There are some unrelated changes once could commit seperatly as NFC and I left some generic code design comments to further simplify this. Though this already looks pretty good to me.



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


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



================
Comment at: llvm/unittests/Analysis/VectorFunctionABITest.cpp:482
+  EXPECT_EQ(Mappings, Exp);
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69976