Page MenuHomePhabricator

[VFABI] Read/Write functions for the VFABI attribute.
ClosedPublic

Authored by fpetrogalli on Thu, Nov 7, 4:15 PM.

Details

Summary

The attribute is stored at the FunctionIndex attribute set, with the name "vector-function-abi-variant".

The get/set methods of the attribute have assertion to verify that:

  1. Each name in the attribute is a valid VFABI mangled name.
  2. Each name in the attribute correspond to a function declared in the module.

Diff Detail

Event Timeline

fpetrogalli created this revision.Thu, Nov 7, 4:15 PM

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.

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?

llvm/lib/Transforms/Utils/ModuleUtils.cpp
317

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())

llvm/unittests/Analysis/VectorFunctionABITest.cpp
482

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.

Thanks for this patch @fpetrogalli, I appreciate you splitting this off from the other patch!
The patch overall looks good to me when you implement some of @jdoerfert suggestions.

llvm/include/llvm/Transforms/Utils/ModuleUtils.h
124

Can these use StringRef?

fpetrogalli marked 7 inline comments as done.Fri, Nov 8, 8:16 AM
fpetrogalli added inline comments.
llvm/include/llvm/Transforms/Utils/ModuleUtils.h
124

Indeed they can. Thank you.

llvm/lib/Analysis/VectorUtils.cpp
1184

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.

llvm/lib/Transforms/Utils/ModuleUtils.cpp
317

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...

llvm/unittests/Analysis/VectorFunctionABITest.cpp
482

Bad coding, sorry. I fixed both tests.

fpetrogalli marked 4 inline comments as done.
fpetrogalli marked 2 inline comments as not done.

@jdoerfert , @sdesmalen , thank you for the reviews!

[spam] Testing arcanist.

fpetrogalli marked an inline comment as done.Fri, Nov 8, 9:25 AM
fpetrogalli added inline comments.
llvm/include/llvm/Transforms/Utils/ModuleUtils.h
124

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.

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

llvm/include/llvm/Transforms/Utils/ModuleUtils.h
124

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.

llvm/lib/Analysis/VectorUtils.cpp
1184

If you need deduplication -> SetVector :)

llvm/lib/Transforms/Utils/ModuleUtils.cpp
286

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.

317

See above :)

fpetrogalli marked 4 inline comments as done.

Minor changes for last review from @jdoerfert .

fpetrogalli marked 3 inline comments as done.Fri, Nov 8, 3:09 PM
sdesmalen added inline comments.Sat, Nov 9, 1:50 PM
llvm/include/llvm/Analysis/VectorUtils.h
138

It would be better passed as an ArrayRef<std::string>, or otherwise a SmallVectorImpl<std::string> &VariantMappings.

llvm/include/llvm/Transforms/Utils/ModuleUtils.h
124

Great, thanks for clarifying!

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

llvm/lib/Analysis/VectorUtils.cpp
1172

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.

llvm/lib/Transforms/Utils/ModuleUtils.cpp
304

nit: unnecessary curly braces.

327

nit: is this assert really necessary?

353

are these asserts necessary?

365

nit: the comment doesn't really clarify more than the code already expresses, so can be removed.

376

if (!TLIName.empty())

378

remove?

391

is this assert necessary?

fpetrogalli marked 17 inline comments as done.Mon, Nov 11, 6:59 AM
fpetrogalli added inline comments.
llvm/lib/Analysis/VectorUtils.cpp
1172

I invoked getModule directly in the assertion, as it is the only place where it is used.

llvm/lib/Transforms/Utils/ModuleUtils.cpp
304

This function shouldn't be here, I have removed it.

327

This method shouldn't be here. I have removed it.

353

This function shouldn't be here. I have removed it.

365

This function shouldn't be here. I have removed it.

376

This function shouldn't be here. I have removed it.

378

This function shouldn't be here. I have removed it.

fpetrogalli marked 7 inline comments as done.

Address review from @sdesmalen .

Notice that I removed a couple of stray methods that where related to the handling of the TLI changes.

This revision is now accepted and ready to land.Mon, Nov 11, 1:48 PM
jdoerfert accepted this revision.Mon, Nov 11, 2:28 PM
This revision was automatically updated to reflect the committed changes.