[PATCH] D87813: [llvm][lld] Add an option to emit cold clusters to a different section.

Thu Sep 17 10:31:44 PDT 2020

tmsriram added inline comments.

================
Comment at: lld/ELF/Writer.cpp:134
   // will be kept in the ".text.unknown" section.
+  // ".text.split." holds symbols which are split out from functions in other
+  // input sections. For example, with -fsplit-machine-functions, placing the
----------------
snehasish wrote:
> MaskRay wrote:
> > 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.
> 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").
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87813/new/

https://reviews.llvm.org/D87813