Page MenuHomePhabricator

[hot-cold-split] Name split functions with ".cold" suffix
ClosedPublic

Authored by tejohnson on Oct 22 2018, 3:49 PM.

Details

Summary

The current default of appending "_"+entry block label to the new
extracted cold function breaks demangling. Change this to ".cold" for hot
cold splitting (other uses of CodeExtractor still use old naming), since
the demangler knows how to handle suffixes starting with ".". This
is also consistent with how gcc names the split cold functions.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson created this revision.Oct 22 2018, 3:49 PM
sebpop added inline comments.Oct 22 2018, 9:09 PM
lib/Transforms/Utils/CodeExtractor.cpp
678 ↗(On Diff #170518)

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.

tejohnson added inline comments.Oct 22 2018, 9:12 PM
lib/Transforms/Utils/CodeExtractor.cpp
678 ↗(On Diff #170518)

Will the hot cold split pass extract more than one cold function from a single function?

sebpop added inline comments.Oct 23 2018, 10:11 AM
lib/Transforms/Utils/CodeExtractor.cpp
678 ↗(On Diff #170518)
tejohnson added inline comments.Oct 23 2018, 10:46 AM
lib/Transforms/Utils/CodeExtractor.cpp
678 ↗(On Diff #170518)

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...

tejohnson added inline comments.Oct 23 2018, 12:27 PM
lib/Transforms/Utils/CodeExtractor.cpp
678 ↗(On Diff #170518)

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).

tejohnson added inline comments.Oct 23 2018, 1:00 PM
lib/Transforms/Utils/CodeExtractor.cpp
678 ↗(On Diff #170518)

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.

Does the pass groups hot code together? is it in your plans?

tejohnson added a subscriber: vsk.Oct 23 2018, 1:10 PM

Does the pass groups hot code together? is it in your plans?

@sebpop, @vsk or @hiraditya can probably answer this. I'm not working on the core hot cold splitting algorithm, just some issues I'm finding as I try it. But presumably the linker should be grouping the split hot functions together and the split cold functions together, when building with -ffunction-sections and PGO which should put them in the .text.hot and .text.unlikely sections, respectively.

tejohnson updated this revision to Diff 170742.Oct 23 2018, 1:36 PM
  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.

Unfortunately change 1) required many test changes.

The change looks good with some minor changes. Thanks!

lib/Transforms/IPO/HotColdSplitting.cpp
345 ↗(On Diff #170742)

Given the commit message: Pass in "cold."+Count
I think the above statement is missing the dot after cold:

/* Suffix */ "cold." + std::to_string(Count));
424 ↗(On Diff #170742)

I like this change, makes it very easy to get unique function names.

test/Transforms/HotColdSplit/split-cold-2.ll
7 ↗(On Diff #170742)

I was expecting to see an integer appended to "cold" like so "fun.cold.1"

tejohnson added inline comments.Oct 24 2018, 9:33 AM
lib/Transforms/IPO/HotColdSplitting.cpp
345 ↗(On Diff #170742)

Indeed, this was missing the ".", fixed.

test/Transforms/HotColdSplit/split-cold-2.ll
7 ↗(On Diff #170742)

Yep, it was passing because of the partial match working. I added the ".1" (which in fact would have caught the oversight you pointed out earlier in the code!).

tejohnson updated this revision to Diff 170918.Oct 24 2018, 9:34 AM

Address comments

vsk added a comment.Oct 24 2018, 9:46 AM

Does the pass groups hot code together? is it in your plans?

@sebpop, @vsk or @hiraditya can probably answer this. I'm not working on the core hot cold splitting algorithm, just some issues I'm finding as I try it. But presumably the linker should be grouping the split hot functions together and the split cold functions together, when building with -ffunction-sections and PGO which should put them in the .text.hot and .text.unlikely sections, respectively.

The pass currently does not group hot or cold code together. Teresa's explanation is correct -- we'll need linker support to do that. On Darwin I have plans to implement this by adding a section modifier (similar to live_support), as this doesn't require creating a new section or messing with order files.

This revision is now accepted and ready to land.Oct 24 2018, 10:02 AM
This revision was automatically updated to reflect the committed changes.