Page MenuHomePhabricator

[VectorUtils] Expose vector-function-abi-variant mangling as a utility. NFC
ClosedPublic

Authored by anna on May 13 2020, 8:11 AM.

Details

Summary

This is an intended NFC. Cleans code up a bit and also exposes the
vector-function-abi-variant mangling as a utility in VectorUtils.
This is needed for downstream or front-ends that add this
attribute.

Diff Detail

Event Timeline

anna created this revision.May 13 2020, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 8:11 AM

Hi @anna

is this change needed so that you can attach the attribute also to llvm::Function and not just llvm::Callinst? I have explained that this is something we don't want to do: https://reviews.llvm.org/D70107#2034194

Let me know if you have any questions.

Kind regards,

Francesco

anna added a comment.May 13 2020, 10:00 AM

Hi @fpetrogalli,
Yes, but it should be orthogonal to that requirement. I think hiding the mangling behind a static function, while the demangling is exposed seems unintuitive. Any front-ends that need to attach the attribute will need this function exposed.

I guess exposing this doesn't hurt anyone and allows fronte-ends to reuse this. LG from my perspective.

anna added a comment.May 13 2020, 12:31 PM

Thanks Johannes. @fpetrogalli Francesco, I believe I've addressed your concern as well?

[...] but it should be orthogonal to that requirement. I think hiding the mangling behind a static function, while the demangling is exposed seems unintuitive. Any front-ends that need to attach the attribute will need this function exposed.

Yep, I fully agree, thank you for explaining!

The function you are exposing is not a generic VFABI mangling function. The VFABI mangling function would take in input the VFShape that we need to realize from a scalar function, and mangle it accordingly setting the correct tokens for each parameter (whether it is linear, uniform... and so on). The method you are exposing is TLI-specific: all the parameter tokens of the mangled name are rendered with "v". Hence, I think that you should leave the name of the method specific to the TLI, as in VFABI::mangleTLIVectorName. Or alternatively, expose it as a static method of the class TargetLibraryInfo.

Also, given that now this is going to be exposed as method like the demangler, I think it would make sense to add some unit tests in llvm/unittests/Analysis/VectorFunctionABITest.cpp (or the TLI tests if you go for the static method of TLI).

Thank you!

Francesco

Francesco

Make sense to expose this API. Seems ok for me conceptually.
+1 for a test and a nit.

llvm/include/llvm/Analysis/VectorUtils.h
190

Since this is for the LLVM-specific VectorABI, why not rename it to mangleLLVMVectorName for less potential confusion?

fpetrogalli added inline comments.May 14 2020, 12:45 PM
llvm/include/llvm/Analysis/VectorUtils.h
190

I was thinking... given that we are talking about exposing an API that would be needed for all target ISA (including the _LLVM_ one), should we just get it done straight from the beginning? Instead of exposing a TLI only method, implement the following:

std::string VFABI::mangleScalarName(StringRef ScalarName, VFISAKind ISA, VFShape Shape);

This would implement the mangling rules described at https://llvm.org/docs/LangRef.html#call-site-attributes, including unit tests for it.

Then for TLI names, all we need to do would be to invoke it (for example in the frontend, and in the InjectTLIMappings, in a separate patch), as follows:

... = VFABI:mangleScalarName(CI->getCalledFunction()->getName(), VFIsaKind::LLVM, VFShape::get(CI, {VF, false}, false))

@anna , is this something you have time to do? If not, I am fine with exposing the TLI specific method, just make sure that the fact that it is TLI specific is mentioned in the name.

@simoll, regarding:

why not rename it to mangleLLVMVectorName for less potential confusion

I see your point, but the LLVM-specific ABI does not prevent using tokens other than "v", so the method as it is now is really TLI specific, not LLVM-ABI specific.

anna marked an inline comment as done.May 14 2020, 1:29 PM
anna added inline comments.
llvm/include/llvm/Analysis/VectorUtils.h
190

I agree that is a good idea @fpetrogalli. Unfortunately, I do not have time for that now. I'll update the patch with the changes you've suggested for the TLI version.

anna updated this revision to Diff 264087.May 14 2020, 1:57 PM

addressed review comments. Added unittest case.

fpetrogalli added inline comments.May 14 2020, 2:35 PM
llvm/include/llvm/Analysis/VectorUtils.h
190

Sure, I understand, no worries!

llvm/unittests/Analysis/VectorFunctionABITest.cpp
94–95

I don't think we need this. The demangler is already tested extensively. If some tests are missing, I think it is better to add them separately as demangler tests.

I think all we need here (in the body of the test below) is just a couple of invocation of the API that makes sure that the correct string is generated. Something like:

EXPECT_EQ(VFABI::mangleTLIVectorName( "vec", "scalar", 3, 4),"_ZGV_LLVM_N4vvv_scalar(vec)");
EXPECT_EQ(VFABI::mangleTLIVectorName( "A", "B", 4, 5), "_ZGV_LLVM_N5vvvv_B(A)");
anna marked an inline comment as done.May 14 2020, 4:38 PM
anna added inline comments.
llvm/unittests/Analysis/VectorFunctionABITest.cpp
94–95

okay, I like that idea of decoupling them. Yeah, just checked - we handle all of these in the demangler case.

anna updated this revision to Diff 264126.May 14 2020, 4:41 PM

addressed review comment about test case.

fpetrogalli accepted this revision.May 15 2020, 5:35 AM

LGTM, with one need that it would be great to fix before submitting to master.

Thank you for working on this!

Francesco

llvm/include/llvm/Analysis/VectorUtils.h
176

Nit: please mention the fact that this method is specific for the TargetLibraryInfo mappings.

This revision is now accepted and ready to land.May 15 2020, 5:35 AM
anna marked an inline comment as done.May 15 2020, 8:42 AM

thank you for the review!

This revision was automatically updated to reflect the committed changes.