This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On Thu, Oct 19, 2017 at 2:45 PM, Cary Coutant <ccoutant@gmail.com> wrote: > + bool text_unlikely_segment > + = (parameters->options().text_unlikely_segment() > + && is_prefix_of(".text.unlikely", > + object->section_name(shndx).c_str())); > + if (it == this->section_segment_map_.end() > + && !text_unlikely_segment) > > To make the code easier to follow, I'd prefer to see this section of > code rewritten into three forks: > > if (text_unlikely_segment) > { > // new code for .text.unlikely ... > } > else > { > Section_segment_map::iterator it = ... > if (it == this->section_segment_map_.end()) > { > os = choose_output_section(...); > } > else > { > // We know the name of the output section... > ... > os = get_output_section(...); > ... > } > } > > - const char* os_name = it->second->name; > + const char* os_name = text_unlikely_segment ? > + ".text.unlikely" : it->second->name; > Done. > The ?: expression needs parens, and the ? should be at the beginning > of the next line, like so: > > const char* os_name = (text_unlikely_segment > ? ".text.unlikely" : it->second->name); > > (This is moot if you follow my suggestion above.) > > if (!os->is_unique_segment()) > { > os->set_is_unique_segment(); > - os->set_extra_segment_flags(it->second->flags); > - os->set_segment_alignment(it->second->align); > + if (!text_unlikely_segment) > + { > + os->set_extra_segment_flags(it->second->flags); > + os->set_segment_alignment(it->second->align); > + } > > Do you want to call set_is_unique_segment() for .text.unlikely > sections? That sounds wrong to me. If that's the right thing to do, > please explain in a comment. Yes, I want the final binary to be like this: Elf file type is EXEC (Executable file) Entry point 0x400560 There are 10 program headers, starting at offset 64 Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flags Align .... LOAD 0x0000000000000000 0x0000000000400000 0x0000000000400000 0x0000000000000970 0x0000000000000970 R E 1000 LOAD 0x0000000000000970 0x0000000000401970 0x0000000000401970 0x000000000000000b 0x000000000000000b R E 1000 LOAD 0x0000000000000de8 0x0000000000402de8 0x0000000000402de8 ... Section to Segment mapping: Segment Sections... ... 02 .interp .note.ABI-tag .note.gnu.build-id .dynsym .dynstr .gnu.hash .hash .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame .eh_frame_hdr 03 .text.unlikely 04 .ctors .dtors .jcr .dynamic .got .got.plt .data .bss One more PT_LOAD segment for .text.unlikely which can only be done with set_is_unique_segment. > > + DEFINE_bool(text_unlikely_segment, options::DASH_Z, '\0', false, > + N_("Move .text.unlikely sections to a separate ELF segment."), > + N_("Do not move .text.unlikely sections to a separate " > + "ELF segment.")); > > Just call it a "segment" (not "ELF segment"). Done. PTAL. Thanks Sri > > -cary > > > On Thu, Oct 19, 2017 at 11:30 AM, Sriraman Tallam <tmsriram@google.com> wrote: >> Ping. Cary, is this alright to commit? >> >> * options.h (-z,text_unlikely_segment): New option. >> * layout.cc (Layout::layout): Create new output section >> for .text.unlikely sections with the new option. >> (Layout::segment_precedes): Check for the new option >> when segment flags match. >> * testsuite/text_unlikely_segment.cc: New test source. >> * testsuite/text_unlikely_segment.sh: New test script. >> * testsuite/Makefile.am (text_unlikely_segment): New test. >> >> On Tue, Oct 10, 2017 at 2:52 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>> Ping. Cary, is this alright to commit? >>> >>> * options.h (-z,text_unlikely_segment): New option. >>> * layout.cc (Layout::layout): Create new output section >>> for .text.unlikely sections with the new option. >>> (Layout::segment_precedes): Check for the new option >>> when segment flags match. >>> * testsuite/text_unlikely_segment.cc: New test source. >>> * testsuite/text_unlikely_segment.sh: New test script. >>> * testsuite/Makefile.am (text_unlikely_segment): New test. >>> >>> >>> >>> >>> On Thu, Oct 5, 2017 at 4:41 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>>> On Thu, Oct 5, 2017 at 4:41 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>> On 10/5/17, Sriraman Tallam <tmsriram@google.com> wrote: >>>>>> On Wed, Oct 4, 2017 at 5:12 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>>>> On 10/4/17, Sriraman Tallam <tmsriram@google.com> wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> This patch adds an option to gold to create a new ELF segment for >>>>>>>> code determined unlikely by the compiler. Currently, this can be done >>>>>>>> with a linker plugin but I was wondering if it is fine to have this >>>>>>>> support in gold with an option for ease of use. >>>>>>>> >>>>>>>> The advantages of doing this are: >>>>>>>> >>>>>>>> * We could only map the hot segment to huge pages to keep iTLB misses >>>>>>>> low >>>>>>>> * We could munlock the unlikely segment >>>>>>>> >>>>>>>> Is this alright? >>>>>>>> >>>>>>>> ChangeLog entry: >>>>>>>> >>>>>>>> * options.h (text_unlikely_segment): New option. >>>>>>>> * layout.cc (Layout::layout): Create new output section >>>>>>>> for .text.unlikely sections with the new option. >>>>>>>> (Layout::segment_precedes): Check for the new option >>>>>>>> when segment flags match. >>>>>>>> >>>>>>>> Patch attached. >>>>>>>> >>>>>>> >>>>>>> This is an interesting approach. Do you have some performace >>>>>>> numbers? >>>>>> >>>>>> With function re-ordering of hot code, segment splitting and mapping >>>>>> only hot code to huge pages, we see a reduction in iTLB misses >>>>>> translating to performance improvements of 0.5 to 1% on some of our >>>>>> benchmarks. >>>>> >>>>> Please include this info in your commit log. >>>>> >>>>>>> >>>>>>> 2 Comments: >>>>>>> >>>>>>> 1. I'd prefer "-z text-unlikely-segment" with '-', instead of '_'. >>>>>>> 2. Need a testcase. >>>>>> >>>>>> Done and patch attached. >>>>>> >>>>> >>>>> LGTM. But I can't approve it. >>>> >>>> Np, thanks! >>>> >>>> Sri >>>> >>>>> >>>>> Thanks. >>>>> >>>>> -- >>>>> H.J.
Attachment:
segment_split.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |