[Bug tree-optimization/45781] New: GCC incorrectly puts function in .text.unlikely

[Bug tree-optimization/45781] New: GCC incorrectly puts function in .text.unlikely

[sje at cup dot hp.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

           Summary: GCC incorrectly puts function in .text.unlikely
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: sje@cup.hp.com


Created attachment 21875
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=21875
Test case

If you compile the attached test case (x.c) with -O2 on ToT sources you will
find that init_target_chars is put into .text.unlikely instead of .text.  I
have verified this on IA64 HP-UX and Linux and on X86 Linux.  I do not think
this function should be put into .text.unlikely in this case and GCC 4.5 does
not do so.

It looks like this started with Honza's partial inlining change (r161382).

The following patch seems to fix the problem but I am not sure if it is the
correct fix or not.  Should node->frequency always be set in this case or
should the original value be preserved if no attribute is seen.  I do not
believe node->frequency is uninitialized because if I change the frequeny
enum in cgraph.h to make NODE_FREQUENCY_NORMAL a 0 value, the frequency
is still set to NODE_FREQUENCY_UNLIKELY_EXECUTED (unless I change predict.c).


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;

-- 
Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.6.0
            Summary|GCC incorrectly puts        |[4.6 Regression] GCC
                   |function in .text.unlikely  |incorrectly puts function
                   |                            |in .text.unlikely

-- 
Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |build
      Known to work|                            |4.5.1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-09-30 10:19:00 UTC ---
The decision is reasonable (I suppose partial inlining will inline the
if (!init) case) as the function is called exactly once then and thus
should be optimized for size and put into a separate section.

The question is thus, why does that break IA64 bootstrap?

If IA64 doesn't support .text.unlikely it should define
UNLIKELY_EXECUTED_TEXT_SECTION_NAME appropriately.

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

--- Comment #2 from Jan Hubicka <hubicka at gcc dot gnu.org> 2010-09-30 16:44:17 UTC ---
IA-64 seems to be fine with unlikely section at least at our periodic tester
setup, otherwise SPEC2000 FDO testing would break. So it might be specific for
ia64 HP-UX and in that case indeed correct fix is to simply define cold and hot
sections to be .text sections (or fix it at binutils side) for HP-UX IA-64 (and
possibly PA target too).

What I am confused about is how partial inlining affect placement of
init_target_chars. If it was because function got partially inlined, the wrong
call would be named init_target_chars.part.XXX and it is not.

It is possible that partial inlining affect profile in some of the callers and
then it might be some bug in profile updating, so I would like to see a
testcase.
x.c in the PR is truncated and when i compile builtins.c from my GCC tree
(x86-64) I do not get init_target_chars in unlikely function.
For some funny reason I however get gimple_rewrite_call_expr. All uses of it
are preceeded by very many exists from the function that makes them appear
cold. Funny but probably not harmful.

Honza

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[sje at cup dot hp.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

--- Comment #3 from Steve Ellcey <sje at cup dot hp.com> 2010-09-30 17:41:36 UTC ---
(In reply to comment #1)
> The decision is reasonable (I suppose partial inlining will inline the
> if (!init) case) as the function is called exactly once then and thus
> should be optimized for size and put into a separate section.

But when I compile with -O2, partial inlining doesn't inline anything.
All the calls still exist as they are in the code.  Actually, the only
reason I can see for moving init_target_chars to .text.unlikely is that
there are 3 'early returns' in fold_builtin_snprintf_chk that could prevent
the execution from ever getting to the init_target_chars call.  Maybe that is
why GCC put it in .text.unlikely.

Very strange, I added:

tree rewrite_call_expr() { return 0; }

to my test case and recompiled to see if this new code went into .text.unlikely
but for some reason that caused everything (including init_target_chars)
to be put into .text.

> 
> The question is thus, why does that break IA64 bootstrap?
> 
> If IA64 doesn't support .text.unlikely it should define
> UNLIKELY_EXECUTED_TEXT_SECTION_NAME appropriately.

Yes, I think I need to define this for IA64 HP-UX.

Re: [Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[Jan Hubicka]

Hi, please, can you add the testcase to PR?  I guess problem might be that as the function is split and then
inlined back together the profile gets confused...

Honza

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[hubicka at ucw dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

--- Comment #4 from Jan Hubicka <hubicka at ucw dot cz> 2010-09-30 17:47:39 UTC ---
Hi, please, can you add the testcase to PR?  I guess problem might be that as
the function is split and then
inlined back together the profile gets confused...

Honza

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

--- Comment #6 from Jan Hubicka <hubicka at gcc dot gnu.org> 2010-09-30 18:21:29 UTC ---
I think I am hitting instance of PR45846.  I get just part of the testcase:
typedef union tree_node *tree;
struct tree_exp { tree operands[1]; };
union tree_node { struct tree_exp exp; };
static unsigned char init_target_chars (void);
static unsigned long long target_newline;



tree rewrite_call_expr()
{
    return 0;
}

tree fold_builtin_snprintf_chk (tree exp)
{
    tree len, fmt;
    if (exp->exp.operands[0] < 5)

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2010.10.14 21:27:58
     Ever Confirmed|0                           |1
           Severity|normal                      |enhancement

--- Comment #7 from Jan Hubicka <hubicka at gcc dot gnu.org> 2010-10-14 21:27:58 UTC ---
Hi,
so bugzilla now gives me whole testcase.  The function is indeed put into
unlikely because there are enough conditionals guarding it.  By the nature of
profile propagation, I guess there is not much to do here, it is just an
estimate after all.
I am putting it as an enhancement as from the nature of code, we probably
should not rate init_target_chars much colder than the folder itself.

Honza

[Bug tree-optimization/45781] [4.6 Regression] GCC incorrectly puts function in .text.unlikely

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45781

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |WONTFIX

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-11-04 11:49:10 UTC ---
Not a bug (as in - works as designed).  The bootstrap problem was in a
different
PR and has been fixed meanwhile.