[lld] Add a new output section ".text.unknown" for funtions with unknown hotness info especially in sampleFDO

[wmi]

Summary

For sampleFDO, because the optimized build uses profile generated from previous release, often we couldn't tell a function without profile was truely cold or just newly created so we had to treat them conservatively and put them in .text section instead of .text.unlikely. The result was when we persue the best performance by locking .text.hot and .text in memory, we wasted a lot of memory to keep cold functions inside. This problem has been largely solved for regular sampleFDO using profile-symbol-list (https://reviews.llvm.org/D66374), but for the case when we use partial profile, we still waste a lot of memory because of it.

In https://reviews.llvm.org/D62540, we propose to save functions with unknown hotness information in a special section called ".text.unknown", so that compiler will treat those functions as luck-warm, but runtime can choose not to mlock the special section in memory or use other strategy to save memory. That will solve most of the memory problem even if we use a partial profile.

The patch adds the support in lld for the special section.

[MaskRay]

Add a new section ".text.unknown"

Add a new *output* section

lld/ELF/Writer.cpp

131

a function

132

a new function

[wmi]

Thanks for looking at it.

lld/ELF/Writer.cpp

131

Fixed.

132

Fixed.

[wmi]

Address MaskRay's comment.

[tmsriram]

lld/ELF/Writer.cpp

135–136

The new prefix looks good to me.

Just curious about input sections that are named just ".text.hot" or ".text.unknown" without the traling period. This could happen with -fno-unique-section-names I think? In that case, this handling seems insufficient. This is related to -keep-text-section-prefix in general. I can take a look at this.

[wmi]

lld/ELF/Writer.cpp

135–136

isSectionPrefix seems considering the section name without trailing period (prefix.drop_back())?

static bool isSectionPrefix(StringRef prefix, StringRef name) {
  return name.startswith(prefix) || name == prefix.drop_back();
}

[tmsriram]

lld/ELF/Writer.cpp

135–136

Ah, thanks!

[tmsriram]

LGTM. You still need an approval from a LLD owner.

[MaskRay]

Looks great!

lld/ELF/Writer.cpp

135–136

I know what you are discussing... (And I mentioned this problem internally previously)

To prevent a linker change (the GNU ld and gold approach has an small annoying behavior), I created https://reviews.llvm.org/D79600

[wmi]

lld/ELF/Writer.cpp

135–136

I am not familiar with linker so I don't know the behavior of GNU ld and gold, but lld seems recognize ".text.hot" and return ".text.hot." for it, so I don't fully understand the intention of D79600, could you explain it a little bit? Thanks.

[MaskRay]

lld/ELF/Writer.cpp

135–136

I updated the description of D79600 a bit. Hope it is clear now... I can clarify if you still think that unclear.

In lld, I hope we only recognize .text.exit.* but not .text.exit, so that the function exit in a function section will not be ordered strangely.

To achieve that goal, we can rename .text.exit (without function sections, or with -fno-unique-section-names) to .text.exit.

[wmi]

lld/ELF/Writer.cpp

135–136

Thanks, now I see what you and Sri mean. Although ".text.hot" is mapped to ".text.hot." when outputing the section, it is not recognized during section ordering or other phases in linker. The function isSectionPrefix is only called by getOutputSectionName so only section outputing phase recognize ".text.hot"

Then D79600 makes sense to me. Thanks for clarifying!

[tmsriram]

lld/ELF/Writer.cpp

135–136

Isn't handling it in the linker the right solution?

Prefixes like ".text.unikely", ".text.hot", etc. are produced by all compilers. That also allows interoperability of various compilers and linkers. LLD can be fixed to handle the prefixes without the trailing period right? I dont have a strong opinion, just my 2 cents.

[MaskRay]

lld/ELF/Writer.cpp

135–136

By all compilers, do you mean GCC? I think we can patch GCC as well (if we still care about the use case) to avoid the aforementioned ambiguity.
I prefer the current way LLD handles things to avoid ambiguity.

[MaskRay]

lld/ELF/Writer.cpp

135–136

Actually, GCC doesn't support -fno-unique-section-names. This requires a GNU as feature which is fairly new addition http://sourceware.org/PR25380 (I raised these feature requests but somehow I forgot this point when making the previous comment...)

[wmi]

D79600 is a followup and the discussion about it won't affect this change, so I will go ahead and submit the change.

lld/ELF/Writer.cpp

if (s->name == "COMMON")
     return ".bss";
 
   if (script->hasSectionsCommand)
     return s->name;
 
   // When no SECTIONS is specified, emulate GNU ld's internal linker scripts
   // by grouping sections with certain prefixes.
 
-  // GNU ld places text sections with prefix ".text.hot.", ".text.unlikely.",
-  // ".text.startup." or ".text.exit." before others. We provide an option -z
-  // keep-text-section-prefix to group such sections into separate output
-  // sections. This is more flexible. See also sortISDBySectionOrder().
+  // GNU ld places text sections with prefix ".text.hot.", ".text.unknown.",
+  // ".text.unlikely.", ".text.startup." or ".text.exit." before others.
+  // We provide an option -z keep-text-section-prefix to group such sections
+  // into separate output sections. This is more flexible. See also
+  // sortISDBySectionOrder().
+  // ".text.unknown" means the hotness of the section is unknown. When
+  // SampleFDO is used, if a function doesn't have sample, it could be very
+  // cold or it could be a new function never being sampled. Those functions
+  // will be kept in the ".text.unknown" section.
   if (config->zKeepTextSectionPrefix)
-    for (StringRef v :
-         {".text.hot.", ".text.unlikely.", ".text.startup.", ".text.exit."})
+    for (StringRef v : {".text.hot.", ".text.unknown.", ".text.unlikely.",
+                        ".text.startup.", ".text.exit."})
       if (isSectionPrefix(v, s->name))
         return v.drop_back();
 
   for (StringRef v :
        {".text.", ".rodata.", ".data.rel.ro.", ".data.", ".bss.rel.ro.",
         ".bss.", ".init_array.", ".fini_array.", ".ctors.", ".dtors.", ".tbss.",
         ".gcc_except_table.", ".tdata.", ".ARM.exidx.", ".ARM.extab."})
     if (isSectionPrefix(v, s->name))

lld/test/ELF/text-section-prefix.s

 # REQUIRES: x86
 ## -z keep-text-section-prefix separates text sections with prefix .text.hot,
-## .text.unlikely, .text.startup, or .text.exit, in the absence of a SECTIONS command.
+## .text.unknown, .text.unlikely, .text.startup, or .text.exit, in the absence
+## of a SECTIONS command.
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
 # RUN: ld.lld %t.o -o %t1
 # RUN: llvm-readelf -S %t1 | FileCheck --check-prefix=NOKEEP %s
 # RUN: ld.lld -z nokeep-text-section-prefix %t.o -o %t2
 # RUN: cmp %t1 %t2
 
 # RUN: ld.lld -z keep-text-section-prefix %t.o -o %t.keep
 # RUN: llvm-readelf -S %t.keep | FileCheck --check-prefix=KEEP %s
 
 # KEEP:      [ 1] .text
 # KEEP-NEXT: [ 2] .text.hot
-# KEEP-NEXT: [ 3] .text.startup
-# KEEP-NEXT: [ 4] .text.exit
-# KEEP-NEXT: [ 5] .text.unlikely
+# KEEP-NEXT: [ 3] .text.unknown
+# KEEP-NEXT: [ 4] .text.startup
+# KEEP-NEXT: [ 5] .text.exit
+# KEEP-NEXT: [ 6] .text.unlikely
 
 # NOKEEP:    [ 1] .text
 # NOKEEP-NOT:     .text
 
 ## With a SECTIONS command, orphan sections are created verbatim.
 ## No grouping is performed for them.
 # RUN: echo 'SECTIONS {}' > %t.lds
 # RUN: ld.lld -T %t.lds -z keep-text-section-prefix %t.o -o %t.script
 # RUN: llvm-readelf -S %t.script | FileCheck --check-prefix=SCRIPT %s
 
 # SCRIPT:      .text
 # SCRIPT-NEXT: .text.f
 # SCRIPT-NEXT: .text.hot.f_hot
+# SCRIPT-NEXT: .text.unknown.f_unknown
 # SCRIPT-NEXT: .text.startup.f_startup
 # SCRIPT-NEXT: .text.exit.f_exit
 # SCRIPT-NEXT: .text.unlikely.f_unlikely
 
 .globl _start
 _start:
   ret
 
 .section .text.f,"ax"
   nop
 
 .section .text.hot.f_hot,"ax"
   nop
 
+.section .text.unknown.f_unknown,"ax"
+  nop
+
 .section .text.startup.f_startup,"ax"
   nop
 
 .section .text.exit.f_exit,"ax"
   nop
 
 .section .text.unlikely.f_unlikely,"ax"
   nop