[PATCH] D66024: [SVFS] Vector Function ABI name demangler.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h:29
+namespace llvm {
+
+/// Describes the type of Parameters
----------------
fpetrogalli wrote:
> 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?
> 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?

That is fine with me (both the VectorFunction or the VF version). It would be nice and consistent to put VF (or VectorFunction) in front of all these names (ParameterKind, ISAKind, ParamType) as done for VectorFunctionShape.


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

https://reviews.llvm.org/D66024