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

[Francesco Petrogalli via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:29
+namespace llvm {
+
+/// Describes the type of Parameters
----------------
aranisumedh wrote:
> 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.
@lebedev.ri, @jdoerfert  I agree that the names `ParameterKind` and `ParamType` are too generic to be exposed in the `llvm` namespace, but I think that moving them in the VABI namespace might not be the right thing to do. These enum and structure are supposed to support other types of parameters of vector function that might not be OpenMP specific or directly related to a vector function ABI specification. This is one of the goals of the RFC submitted for the SVFS.

Therefore I suggest to move them back to the llvm namespace, with names that will make it clar that we are talking about function vectorization.

How about the following?

`ParameterKind` -> `VectorFunctionParameterKind` or `VFParameterKind`
`ParamType` -> `VectorFunctionParameter` or `VFParameter` (we might want get rid of the `Type` token too)

Does that make sense to you?


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

https://reviews.llvm.org/D66024