[PATCH] D79867: [VectorUtils] Expose vector-function-abi-variant mangling as a utility. NFC

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:184
+/// <vectorname> = the name of the vector function.
+std::string mangleVectorName(StringRef VectorName, StringRef ScalarName,
+                             unsigned numArgs, unsigned VF);
----------------
anna wrote:
> fpetrogalli wrote:
> > simoll wrote:
> > > Since this is for the LLVM-specific VectorABI, why not rename it to `mangleLLVMVectorName` for less potential confusion?
> > 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.
> 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.
Sure, I understand, no worries!


================
Comment at: llvm/unittests/Analysis/VectorFunctionABITest.cpp:94-95
+        VectorNameArg, ScalarNameArg, NumArgs, VFArg);
+    // Testing if the mangledname that is generated can be demangled and the
+    // correct values are retrieved.
+    reset(VectorNameArg, IRType);
----------------
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)");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79867