Re: Gold linker patch to split unlikely text into a separate segment

[Cary Coutant <ccoutant at gmail dot com>]

+      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;

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.

+  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").

-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.