Patchwork Patch for partial inlining/text.unlikely bug, PR tree-optimization/45781

login
register
mail settings
Submitter Steve Ellcey
Date Sept. 29, 2010, 9:56 p.m.
Message ID <201009292156.o8TLu9M07022@lucas.cup.hp.com>
Download mbox | patch
Permalink /patch/66100/
State New
Headers show

Comments

Steve Ellcey - Sept. 29, 2010, 9:56 p.m.
I have had bootstrap problems in IA64 ever since the partial inlining
patch was applied (r161382).  I first reported this in PR44716 and then
later found that the problem was related to the fact that GCC was
putting some code in .text.unlikely (instead of .text) which it never
did before.  This is reported in PR45781.  PR45781 includes a test case
for which this behaviour can be duplicated on x86 as well as IA64 and
probably other platforms as well.

This proposed patch changes compute_function_frequency so that if there
is no branch probabilities or profile info or attributes then a function
is put into the normal text segment by setting node->frequency to
NODE_FREQUENCY_NORMAL instead of not setting at all.

Tested on IA64 HP-UX and Linux and on x86 Linux.

Does this patch seem like the right fix for this problem?

OK to checkin?

Steve Ellcey
sje@cup.hp.com



2010-09-29  Steve Ellcey  <sje@cup.hp.com>

	PR tree-optimization/45781
	PR middle-end/44716
	* predict.c: Set frequency to normal if no overrides.
Jan Hubicka - Sept. 29, 2010, 10:43 p.m.
> I have had bootstrap problems in IA64 ever since the partial inlining
> patch was applied (r161382).  I first reported this in PR44716 and then
> later found that the problem was related to the fact that GCC was
> putting some code in .text.unlikely (instead of .text) which it never
> did before.  This is reported in PR45781.  PR45781 includes a test case
> for which this behaviour can be duplicated on x86 as well as IA64 and
> probably other platforms as well.
> 
> This proposed patch changes compute_function_frequency so that if there
> is no branch probabilities or profile info or attributes then a function
> is put into the normal text segment by setting node->frequency to
> NODE_FREQUENCY_NORMAL instead of not setting at all.
> 
> Tested on IA64 HP-UX and Linux and on x86 Linux.
> 
> Does this patch seem like the right fix for this problem?
> 
> OK to checkin?

Hmm, the frequency is initialized to NODE_FREQUENCY_NORMAL in cgraph_create_node
and thus it should be set so unless someone decides to change it.  Do you have
any idea why the frequency is set to UNLIKELY_EXECUTED?

The reason I ask is because ipa-profile pass might change the frequency on
purpose (if all callers of the function appears to be cold) and since
compute_function_frequency is re-done from fixup_cfg (to compensate frequency
scaling when using profile feedback), this patch will just disable effect of
that pass.

The testcase in PR45781 appears truncated, if you will send me full one, I will
try to take a look first.

Honza
Steve Ellcey - Sept. 29, 2010, 10:49 p.m.
On Thu, 2010-09-30 at 00:43 +0200, Jan Hubicka wrote:

> The testcase in PR45781 appears truncated, if you will send me full one, I will
> try to take a look first.
> 
> Honza

The test case is cut down from gcc/builtins.c.

Steve Ellcey
sje@cup.hp.com
Andi Kleen - Sept. 30, 2010, 3:22 a.m.
Steve Ellcey <sje@cup.hp.com> writes:

> I have had bootstrap problems in IA64 ever since the partial inlining
> patch was applied (r161382).  I first reported this in PR44716 and then
> later found that the problem was related to the fact that GCC was
> putting some code in .text.unlikely (instead of .text) which it never
> did before.  This is reported in PR45781.  PR45781 includes a test case
> for which this behaviour can be duplicated on x86 as well as IA64 and
> probably other platforms as well.


Wouldn't it be better to fix whatever problem IA64 has with
.text.unlikely?

This seems at best a poor workaround for a different bug.

-Andi
Steve Ellcey - Sept. 30, 2010, 4:37 p.m.
On Thu, 2010-09-30 at 00:43 +0200, Jan Hubicka wrote:

> Hmm, the frequency is initialized to NODE_FREQUENCY_NORMAL in cgraph_create_node
> and thus it should be set so unless someone decides to change it.  Do you have
> any idea why the frequency is set to UNLIKELY_EXECUTED?

No, I don't know who is setting it to UNLIKELY_EXECUTED or why.

> The reason I ask is because ipa-profile pass might change the frequency on
> purpose (if all callers of the function appears to be cold) and since
> compute_function_frequency is re-done from fixup_cfg (to compensate frequency
> scaling when using profile feedback), this patch will just disable effect of
> that pass.

It seems odd to me that we never used .text.unlikely before (unless
profiling), but that partial inlining caused us to start using it.  I
think I am going to have to change IA64 HP-UX to not use .text.unlikely
because there seems to be a (HP-UX) linker bug when we call a function
in .text from a function in .text.unlikely.  But regardless of whether
or not I change IA64 HP-UX I think it would be good for us to understand
why we are now using .text.unlikely and if it is accidental or on
purpose.  I don't know if this change is intentional or not but it could
affect other platforms because UNLIKELY_EXECUTED_TEXT_SECTION_NAME is
set to .text.unlikely in defaults.h.

I also notice that the .text.unlikely and .text.hot sections seem to be
handled very differently then all other sections.  I.e. We have
text_section, bss_section, etc variables in varasm.c but no
text_unlikely_section variable.  We set text_section and bss_section
based on TEXT_SECTION_ASM_OP and BSS_SECTION_ASM_OP in init_varasm_once
but the cold text section is set based on
UNLIKELY_EXECUTED_TEXT_SECTION_NAME in initialize_cold_section_name and
is is not clear to me that HOT_TEXT_SECTION_NAME is handled at all.  It
seems like these sections should be handled in a similar manner to all
the other sections but they are not.

Steve Ellcey
sje@cup.hp.com
Jan Hubicka - Sept. 30, 2010, 4:51 p.m.
> On Thu, 2010-09-30 at 00:43 +0200, Jan Hubicka wrote:
> 
> > Hmm, the frequency is initialized to NODE_FREQUENCY_NORMAL in cgraph_create_node
> > and thus it should be set so unless someone decides to change it.  Do you have
> > any idea why the frequency is set to UNLIKELY_EXECUTED?
> 
> No, I don't know who is setting it to UNLIKELY_EXECUTED or why.
> 
> > The reason I ask is because ipa-profile pass might change the frequency on
> > purpose (if all callers of the function appears to be cold) and since
> > compute_function_frequency is re-done from fixup_cfg (to compensate frequency
> > scaling when using profile feedback), this patch will just disable effect of
> > that pass.
> 
> It seems odd to me that we never used .text.unlikely before (unless
> profiling), but that partial inlining caused us to start using it.  I
> think I am going to have to change IA64 HP-UX to not use .text.unlikely
> because there seems to be a (HP-UX) linker bug when we call a function
> in .text from a function in .text.unlikely.  But regardless of whether

I just added coment to the corresponding PR.  We now have ipa-profile pass that
attempts to put into .unlikely sections functions that are only called on very
infrequent places.  It is possible that this does match for the function in
question, but partial inlining should not terribly affect the profiles, so I
would like to understand why this happen.

> or not I change IA64 HP-UX I think it would be good for us to understand
> why we are now using .text.unlikely and if it is accidental or on
> purpose.  I don't know if this change is intentional or not but it could
> affect other platforms because UNLIKELY_EXECUTED_TEXT_SECTION_NAME is
> set to .text.unlikely in defaults.h.

Well, all platforms that breaks with placing function into .text.unlikely either
must be fixed or the default needs to be redefined to .text section.
> 
> I also notice that the .text.unlikely and .text.hot sections seem to be
> handled very differently then all other sections.  I.e. We have
> text_section, bss_section, etc variables in varasm.c but no
> text_unlikely_section variable.  We set text_section and bss_section
> based on TEXT_SECTION_ASM_OP and BSS_SECTION_ASM_OP in init_varasm_once
> but the cold text section is set based on
> UNLIKELY_EXECUTED_TEXT_SECTION_NAME in initialize_cold_section_name and
> is is not clear to me that HOT_TEXT_SECTION_NAME is handled at all.  It
> seems like these sections should be handled in a similar manner to all
> the other sections but they are not.

Well, the code is supposed to simply sed named section attribute for the function
in choose_function_section.
I am not quite sure what initialize_cold_section_name is about, will have to look
more into that code. I originally introduced these sections few years back,
but I don't think this function is of my origin.

Honza
> 
> Steve Ellcey
> sje@cup.hp.com

Patch

Index: predict.c
===================================================================
--- predict.c	(revision 164573)
+++ predict.c	(working copy)
@@ -2204,6 +2204,8 @@  compute_function_frequency (void)
       else if (DECL_STATIC_CONSTRUCTOR (current_function_decl)
 	       || DECL_STATIC_DESTRUCTOR (current_function_decl))
         node->frequency = NODE_FREQUENCY_EXECUTED_ONCE;
+      else 
+        node->frequency = NODE_FREQUENCY_NORMAL;
       return;
     }
   node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;