Page MenuHomePhabricator

[llvm][lld] Add an option to emit cold clusters to a different section.
Needs ReviewPublic

Authored by snehasish on Wed, Sep 16, 9:54 PM.

Details

Summary

This change adds an option to basic block sections to allow cold
clusters to be assigned a custom text prefix. With a custom prefix such
as ".text.split.", lld can place them in a separate output section.
The benefits are -

  • Empirically shown to improve icache and itlb metrics by 3-5% (absolute) compared to placing split parts in .text.unlikely.
  • Mitigates against poor profiles, eg samplePGO profiles used with the machine function splitter. Optimizations such as hugepage remapping can make different decisions at the section granularity.
  • Enables section granularity hotness monitoring (checking on the decisions made during compilation vs sample data from production).

Diff Detail

Unit TestsFailed

TimeTest
50 mswindows > LLVM-Unit.Demangle/_/DemangleTests_exe::Demangle.demangleTest
80 mswindows > LLVM.Other::change-printer.ll
Script: -- : 'RUN: at line 6'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\opt.exe -S -print-changed -passes=instsimplify 2>&1 -o /dev/null < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Other\change-printer.ll | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Other\change-printer.ll --check-prefix=CHECK_SIMPLE

Event Timeline

snehasish created this revision.Wed, Sep 16, 9:54 PM
snehasish requested review of this revision.Wed, Sep 16, 9:54 PM
MaskRay added inline comments.Wed, Sep 16, 10:57 PM
lld/ELF/Writer.cpp
134 ↗(On Diff #292405)

I think I will need to read your first LLVM patch to get some basic understanding of machine function splitter. I'd suggest you split the lld patch from the codegen one.

For your future lld patch: why can't the split sections be placed in .test.cold? This just affects how -z keep-text-section-prefix groups input sections into output sections. With the additional element, there may be an output section .text.split but if its purpose is similar to the others I am not sure you need it.

Drop lld/ELF/Writer.cpp changes.

  • Moved the changes to D87840.
lld/ELF/Writer.cpp
134 ↗(On Diff #292405)

Thanks, here's a link to the RFC for your reference: https://groups.google.com/g/llvm-dev/c/RUegaMg-iqc/m/wFAVxa6fCgAJ
I've split out the LLD change as a separate patch: D87840

I'll add the discussion for the rationale behind a new output section in the D87840 (assuming that you mean ".text.unlikely" instead of ".test.cold").

snehasish edited the summary of this revision. (Show Details)Thu, Sep 17, 10:18 AM
tmsriram added inline comments.Thu, Sep 17, 10:31 AM
lld/ELF/Writer.cpp
134 ↗(On Diff #292405)

A couple of thoughts on this which you could help clarify:

  • Currently, known cold code is put into .text.unlikely and lukewarm stuff is put into .text.
  • Hugepage mappers that I am aware of avoid mapping .text.unlikely into hugepages for max hugepage itlb utilization.
  • For function splitting, when you play with the thresholds more there is a good chance you are going to end up splitting code that is lukewarm and .text.unlikely is not the ideal place for it.
  • In which case, such code can be put into .text itself. Is there a good reason to put this into .text.split? Does having a new output section give you more leverage on how to manage the mapping of such code? I think this is the part that is not fully clear to me. Thanks.