[PATCH] D96109: Refactor implementation of -funique-internal-linkage-names.

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1251
                                       ->getMostRecentDecl();
     std::string OtherName = getMangledNameImpl(*this, OtherGD, OtherFD);
     // This is so that if the initial version was already the 'default'
----------------
rnk wrote:
> tmsriram wrote:
> > rnk wrote:
> > > This raises the original concern that I had that pushed me in the direction of doing things as a pass: How does this interact with multi-version functions? It probably just works, but I remember this was something that made me think an IR pass would be simpler, or more likely to do the right thing.
> > > 
> > > Something else to think about: clang has other ways to generate internal functions with non-unique names. Dynamic initialization comes to mind (`__cxx_global_var_init`). Consider:
> > > ```
> > > template <typename T>
> > > struct Foo { static int gv; };
> > > int f(int);
> > > template <typename T>
> > > int Foo<T>::gv = f(sizeof(T));
> > > void escape(int*);
> > > void instantiate() {
> > >   escape(&Foo<int>::gv);
> > >   escape(&Foo<short>::gv);
> > > }
> > > ```
> > > The IR pass was renaming these for us previously.
> > I had a test for multi versioned functions, clang/test/CodeGen/unique-internal-linkage-names.cpp.  Here is the gist though.   foo() with multi-version clones for sse4.2 and avx will produce the following function symbols:
> > 
> > 1) _Z3foov.__uniq.1234
> > 2) _Z3foov.sse4.2.__uniq.1234
> > 3) _Z3foov.avx.__uniq.1234
> > 4) _Z3foov.__uniq.1234.resolver
> > 
> > Multi-versioned suffixes are not demangler friendly in general as such, even without this patch.  For instance,  _Z3foov.sse4.2 will not demangle,  Is there a specific concern?
> > 
> > 
> > 
> > 
> > 
> Mainly, I'd just like to keep that test coverage. The logic moved to the frontend, so now that should become a frontend test.
> 
> Just from reading the patch, I'm guessing the names now become `_Z3foov.__uniq.1234.avx/sse/etc`, but I could be wrong. It would be good to encode our expectations one way or another.
I  like the module hash coming before the multi-version target suffix too like  you pointed out.  I just need to re-order the code in getMangledNameImpl. I will do that.


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

https://reviews.llvm.org/D96109