This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Process fragment siblings in lite mode, keep lite mode on
ClosedPublic

Authored by Amir on Dec 20 2022, 10:49 PM.

Details

Summary

In lite mode, include split function fragments to the list of functions to
process even if a fragment has no samples. This is required to properly
detect and update split jump tables (jump tables that contain pointers to code
in the main and cold fragments).

Diff Detail

Event Timeline

Amir created this revision.Dec 20 2022, 10:49 PM
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Amir requested review of this revision.Dec 20 2022, 10:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 10:49 PM
maksfb added a comment.Jan 8 2023, 6:48 PM

If the function is local, it will have a /<N> suffix inserted after the main fragment name and after .cold. Will this change cover such case? Please add a test for it.

bolt/lib/Rewrite/RewriteInstance.cpp
1525–1526
Amir updated this revision to Diff 487634.Jan 9 2023, 6:15 PM

Refactoring, bugfixes, added tests

maksfb added a comment.Jan 9 2023, 6:38 PM

What do you think about preliminary matching fragments while reading the symbol table for non-stripped binaries? It's almost exactly what you are doing in this diff, right?

bolt/test/X86/fragment-lite-reverse.s
48

How is foo different from main for the purpose of this test?

From what I've seen, cold fragments typically have local visibility (but we shouldn't rely on it), while main fragments could be of any type.

Amir updated this revision to Diff 487697.Jan 9 2023, 11:35 PM
Amir marked an inline comment as done.

Update testcase

Amir marked an inline comment as done.Jan 9 2023, 11:39 PM

What do you think about preliminary matching fragments while reading the symbol table for non-stripped binaries? It's almost exactly what you are doing in this diff, right?

Let me look into that. I thought that selectFunctionsToProcess is the appropriate place since at that point we know which functions we want to skip and it's the earliest place to override that.

bolt/test/X86/fragment-lite-reverse.s
48

Fixed.

Amir marked an inline comment as done.Jan 9 2023, 11:40 PM

What do you think about preliminary matching fragments while reading the symbol table for non-stripped binaries? It's almost exactly what you are doing in this diff, right?

Let me look into that. I thought that selectFunctionsToProcess is the appropriate place since at that point we know which functions we want to skip and it's the earliest place to override that.

Sorry, I wasn't clear. I mean to match fragments early and then use the fragment dependency info in selectFunctionsToProcess(). It's still the right place to set functions as ignored.

We need another set of tests where the main fragment has local visibility.

Amir updated this revision to Diff 488458.Jan 11 2023, 7:41 PM

Add RI::registerFragments, update tests

Amir updated this revision to Diff 488477.Jan 11 2023, 10:19 PM

Allow multiple parents per fragment (the case of ICF-folded cold fragments)

Amir updated this revision to Diff 488479.Jan 11 2023, 10:33 PM

clang-format

Thanks for refactoring the code.

What happens to stripped binaries? Should we disable lite mode for them?

bolt/lib/Core/BinaryContext.cpp
504

Do we have to check isFragment() before checking isParentFragment()? BTW, what do you think about renaming the latter to isChildOf()?

bolt/lib/Rewrite/RewriteInstance.cpp
737

Add the call to the end of discoverFileObjects(). That's where we register functions, data objects, etc.

1347
1351

What if you have global foo+foo.cold.1 and local foo/1+foo.cold.1/1? What will foo.cold.1/1 match to?

Amir added a comment.Jan 26 2023, 2:07 PM

Thanks for refactoring the code.

What happens to stripped binaries? Should we disable lite mode for them?

Actually, I don't know what happens to stripped binaries. For them, the more functions we process, the higher the risk that we miss a fragment or incorrectly update the jump table. Disabling lite mode doesn't fix processing stripped binaries with split functions. BTW, lite mode wasn't even disabled for such binaries since the determination is based on symbol names.

Amir marked an inline comment as done.
Amir added inline comments.
bolt/lib/Core/BinaryContext.cpp
504
Amir updated this revision to Diff 492599.Jan 26 2023, 4:43 PM
Amir marked an inline comment as done.

Updated fragment-lite test with local and global baz+fragment

Amir marked 3 inline comments as done.Jan 26 2023, 4:44 PM
Amir added a comment.Jan 26 2023, 9:19 PM

Thanks for refactoring the code.

What happens to stripped binaries? Should we disable lite mode for them?

What happens to stripped binaries: D142686

maksfb added inline comments.Feb 3 2023, 5:25 PM
bolt/test/X86/fragment-lite.s
30

IIRC, FileCheck will pass this check even for Parent : baz/1.

Amir updated this revision to Diff 495359.Feb 6 2023, 7:52 PM

Update test

Amir marked an inline comment as done.Feb 6 2023, 7:53 PM
maksfb accepted this revision.Feb 8 2023, 3:00 PM

LGTM

This revision is now accepted and ready to land.Feb 8 2023, 3:00 PM