[PATCH] D66024: Name Demangling as specified in the Vector Function ABI

[Sumedh Arani via Phabricator via llvm-commits]

aranisumedh marked an inline comment as done.
aranisumedh added inline comments.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:29
+namespace llvm {
+
+/// Describes the type of Parameters
----------------
jdoerfert wrote:
> aranisumedh wrote:
> > lebedev.ri wrote:
> > > All these are currently in llvm namespace, would it be better to narrow it and put them into `VFABI`?
> > Someone correct me if I am wrong. 
> > 
> > This module is intended to be extensible to other vectorization as well (as stated in the RFC). I am assuming that it can be extended to other ParameterKind as well. 
> > 
> > Whereas the name demangling for now is as stated in the vector function ABI and hence a part of the VFABI namespace.
> I'm with @lebedev.ri, `ParameterKind` and `ParamType` are such generic names that one can easily see errors in the future of someone "seeing" that definition but not the intended one. What is the problem with wrapping this in the VFABI namespace even if we use it in other contexts?
I see the point you're trying to make. I'll wrap the `ParameterKind` and `ParamType` in the same namespace as the rest.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:149
+/// \param Result True if the MangledName follows the ABI signature
+bool getABISignature(StringRef MangledName);
+/// Gets Instruction Set Architecture
----------------
sdesmalen wrote:
> I don't think we want to expose any of these interfaces directly, as they are only relevant to the parsing of the mangled name vis-a-vis the construction of the VectorFunctionShape object. After it has been parsed, the VectorFunctionShape object can be queried directly for these properties.
> 
> If you make them private methods to VectorFunctionShape, then a constructor of VectorFunctionShape can take `StringRef MangledName` and parse the string to fill out the object, calling all these interfaces under the hood. For this reason, I think you'll want to move `demangleName` and `getParameters` from D66025 to this patch as well.
> 
> I'd also recommend renaming these functions to use 'parse' instead of 'get', and instead of returning the value that they have parsed directly, you can wrap it in a `Optional<>`, so you can check whether an issue occurred while parsing the mangled name.

> I'd also recommend renaming these functions to use 'parse' instead of 'get', 

Will do. 

> and instead of returning the value that they have parsed directly, you can wrap it in a Optional<>, so you can check whether an issue occurred while parsing the mangled name.

I have used asserts for error handling. Also, vector function ABI doesn't allow any fields to be optional. Any mismatch is a failure. 





Repository:
  rL LLVM

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

https://reviews.llvm.org/D66024