| Message ID | 201009292156.o8TLu9M07022@lucas.cup.hp.com |
|---|---|
| State | New |
| Headers | show |
> 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
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
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
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
> 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
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;