[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D73307#1989924 <https://reviews.llvm.org/D73307#1989924>, @erichkeane wrote:

> In D73307#1989740 <https://reviews.llvm.org/D73307#1989740>, @rnk wrote:
>
> > In D73307#1978140 <https://reviews.llvm.org/D73307#1978140>, @tmsriram wrote:
> >
> > > In D73307#1972388 <https://reviews.llvm.org/D73307#1972388>, @rnk wrote:
> > >
> > > > Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?
> > >
> > >
> > > I can try doing this.  If my understanding is right,  you are suggesting that doing this later would mean the alias would have already been applied?.  However, for multi-versioning symbols, I must parse the target name to insert the module name in between right?
> >
> >
> >
>




>> Yes, Clang will apply the alias, do all name mangling, etc, and then renaming will happen later.
>> 
>> It's not clear to me if the order of the `.<target>` suffix on multi-versioned symbols matters. @erichkeane, should it? Supposing some mid-level optimization came along and did function versioning, it would rename the internal function and append `.1`. So, I think appending as you implemented is fine.
>> 
>>  ---
>> 
>> All of my concerns have been addressed, and I think everyone else's are as well. We have documentation indicating what we mean by uniqueness which @hubert.reinterpretcast wanted clarified, we had the prefix map issue which I believe is addressed, so to my knowledge this is ready.
>> 
>> Let's wait a bit to hear from Erich, but otherwise I think this is ready in a few days.
> 
> Multiversioning doesn't care about the names, simply that they are unique/unique from eachother.  The suffix that GCC/ICC put together for these two types is simply to make sure we don't get something that conflicts with another symbol.  As long as these symbols are unique (and correclty updated!) , they can be named absolutely anything.



In D73307#1989924 <https://reviews.llvm.org/D73307#1989924>, @erichkeane wrote:

> In D73307#1989740 <https://reviews.llvm.org/D73307#1989740>, @rnk wrote:
>
> > In D73307#1978140 <https://reviews.llvm.org/D73307#1978140>, @tmsriram wrote:
> >
> > > In D73307#1972388 <https://reviews.llvm.org/D73307#1972388>, @rnk wrote:
> > >
> > > > Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?
> > >
> > >
> > > I can try doing this.  If my understanding is right,  you are suggesting that doing this later would mean the alias would have already been applied?.  However, for multi-versioning symbols, I must parse the target name to insert the module name in between right?
> >
> >
> > Yes, Clang will apply the alias, do all name mangling, etc, and then renaming will happen later.
> >
> > It's not clear to me if the order of the `.<target>` suffix on multi-versioned symbols matters. @erichkeane, should it? Supposing some mid-level optimization came along and did function versioning, it would rename the internal function and append `.1`. So, I think appending as you implemented is fine.
> >
> >  ---
> >
> > All of my concerns have been addressed, and I think everyone else's are as well. We have documentation indicating what we mean by uniqueness which @hubert.reinterpretcast wanted clarified, we had the prefix map issue which I believe is addressed, so to my knowledge this is ready.
> >
> > Let's wait a bit to hear from Erich, but otherwise I think this is ready in a few days.
>
>
> Multiversioning doesn't care about the names, simply that they are unique/unique from eachother.  The suffix that GCC/ICC put together for these two types is simply to make sure we don't get something that conflicts with another symbol.  As long as these symbols are unique (and correclty updated!) , they can be named absolutely anything.


Thanks for the comments!  I was one of the authors of the GCC multiversioning patch, and if I recall correctly from years ago, one of the reasons for naming the target clones of multi-versioned foo as foo, foo.sse, etc. was so that c++filt works nicely on this.  Like, c++filt on _Z3foov.see would demangle as foo() [clone sse].  But c++filt  is  broken IMO as it is not handling these suffixes correctly any more, like c++filt _Z3foov.sse4.2 does not even demangle as expected.


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

https://reviews.llvm.org/D73307