[PATCH] D53534: [hot-cold-split] Name split functions with ".cold" suffix

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:678
+      Suffix.empty() ? oldFunction->getName() + "_" + header->getName()
+                     : oldFunction->getName() + Suffix,
+      M);
----------------
tejohnson wrote:
> tejohnson wrote:
> > sebpop wrote:
> > > tejohnson wrote:
> > > > sebpop wrote:
> > > > > In the case of more than one region extracted from a function, this code may create functions with identical names which would create link problems.
> > > > > 
> > > > Will the hot cold split pass extract more than one cold function from a single function?
> > > Yes with https://reviews.llvm.org/D53588
> > Ok I see. It would make sense then to append a unique id, which could be the header name. Also we may in general want to change the "_" here to a "." so that the demangler handles it appropriately, regardless of whether we change the suffix after that deliminator.
> > 
> > I did see in my internal project that a lot of the cold split funcs actually ended with "_" without any header label - I need to go back and investigate - do we have empty header names in some cases? Because if so, with splitting into multiple funcs we will also end up with name collisions. Let me take another look...
> > 
> > 
> When the IR is generated by a release built clang, the bb labels are empty. So that will cause issues with your D53588 patch, since all outlined funcs will be named original name + "_". So they need to use some other id when the name is empty. Perhaps the client of CodeExtractor needs to supply a unique id (for hot cold split you could just enumerate the split out functions for each original function and use that as a unique id).
Ok I reworked the patch a bit:
1) Always use "." as the delimiter for the suffix instead of "_" to enable demangling
2) If the label is empty and no suffix passed in from client, use "extracted" after the "." (the demangler needs something after the "." to handle it correctly).
3) Pass in "cold."+Count from the hot cold splitting pass as the suffix, where Count is currently hardwired to 1 but after D53588 can be changed to be a monotonically increasing count of the functions split from a single function.

I will upload the patch momentarily.

I just realized though, that we will still have problems if the hot cold split pass is ever run multiple times, which after D53437 is only an issue with the ThinLTO newPM. I will send a separate patch to fix that, that will need to land before this one.


Repository:
  rL LLVM

https://reviews.llvm.org/D53534