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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
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.
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? |
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:
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. |
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. |
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)"); |
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. |
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. |
Nit: please mention the fact that this method is specific for the TargetLibraryInfo mappings.