public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
@ 2013-10-14  8:22 dominiq at lps dot ens.fr
  2013-10-14  8:59 ` [Bug ipa/58721] " rguenth at gcc dot gnu.org
                   ` (25 more replies)
  0 siblings, 26 replies; 27+ messages in thread
From: dominiq at lps dot ens.fr @ 2013-10-14  8:22 UTC (permalink / raw)
  To: gcc-bugs

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

            Bug ID: 58721
           Summary: [4.9 Regression] The subroutine perdida is no longer
                    inlined in fatigue.f90
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dominiq at lps dot ens.fr
                CC: jh at suse dot cz, rguenther at suse dot de

The subroutine perdida is no longer inlined in fatigue.f90. This occurred
between revisions 201916 and 202776:

[macbook] lin/test% /opt/gcc/gcc4.9p-201916/bin/gfortran -Ofast -fwhole-program
fatigue.f90
[macbook] lin/test% nm a.out | grep perdida
[macbook] lin/test% time a.out > /dev/null
2.767u 0.003s 0:02.77 99.6%    0+0k 0+2io 0pf+0w
[macbook] lin/test% /opt/gcc/gcc4.9p-202776/bin/gfortran -Ofast -fwhole-program
fatigue.f90
[macbook] lin/test% nm a.out | grep perdida
0000000100001380 t ___perdida_m_MOD_perdida.constprop.3
[macbook] lin/test% time a.out > /dev/null
5.102u 0.004s 0:05.10 100.0%    0+0k 0+2io 0pf+0w

To understand why I could not bisect further, see
http://gcc.gnu.org/ml/gcc-regression/2013-09/ !-(


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
@ 2013-10-14  8:59 ` rguenth at gcc dot gnu.org
  2013-11-05 15:02 ` rguenth at gcc dot gnu.org
                   ` (24 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: rguenth at gcc dot gnu.org @ 2013-10-14  8:59 UTC (permalink / raw)
  To: gcc-bugs

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|rguenther at suse dot de           |hubicka at gcc dot gnu.org
          Component|tree-optimization           |ipa
   Target Milestone|---                         |4.9.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
  2013-10-14  8:59 ` [Bug ipa/58721] " rguenth at gcc dot gnu.org
@ 2013-11-05 15:02 ` rguenth at gcc dot gnu.org
  2013-11-28 14:01 ` ysrumyan at gmail dot com
                   ` (23 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: rguenth at gcc dot gnu.org @ 2013-11-05 15:02 UTC (permalink / raw)
  To: gcc-bugs

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
           Priority|P3                          |P1


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
  2013-10-14  8:59 ` [Bug ipa/58721] " rguenth at gcc dot gnu.org
  2013-11-05 15:02 ` rguenth at gcc dot gnu.org
@ 2013-11-28 14:01 ` ysrumyan at gmail dot com
  2013-11-28 19:19 ` hjl.tools at gmail dot com
                   ` (22 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: ysrumyan at gmail dot com @ 2013-11-28 14:01 UTC (permalink / raw)
  To: gcc-bugs

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

Yuri Rumyantsev <ysrumyan at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ysrumyan at gmail dot com

--- Comment #1 from Yuri Rumyantsev <ysrumyan at gmail dot com> ---
We found the cause of performance degradation - bug was introduced by r202567,
namely in callback function "check_callers":

was

if (!has_hot_call && cgraph_maybe_hot_edge_p (e))

must be

if (!(*(bool*)has_hot_call) && cgraph_maybe_hot_edge_p (e))

After apllying this fix routine is inlined.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (2 preceding siblings ...)
  2013-11-28 14:01 ` ysrumyan at gmail dot com
@ 2013-11-28 19:19 ` hjl.tools at gmail dot com
  2013-11-28 23:04 ` dominiq at lps dot ens.fr
                   ` (21 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: hjl.tools at gmail dot com @ 2013-11-28 19:19 UTC (permalink / raw)
  To: gcc-bugs

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-11-28
                 CC|                            |hjl.tools at gmail dot com
     Ever confirmed|0                           |1

--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Yuri Rumyantsev from comment #1)
> We found the cause of performance degradation - bug was introduced by
> r202567, namely in callback function "check_callers":
> 
> was
> 
> if (!has_hot_call && cgraph_maybe_hot_edge_p (e))
> 
> must be
> 
> if (!(*(bool*)has_hot_call) && cgraph_maybe_hot_edge_p (e))
> 
> After apllying this fix routine is inlined.

It looks obvious.  Please submit a patch.  Thanks.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (3 preceding siblings ...)
  2013-11-28 19:19 ` hjl.tools at gmail dot com
@ 2013-11-28 23:04 ` dominiq at lps dot ens.fr
  2013-12-02 12:12 ` ysrumyan at gmail dot com
                   ` (20 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: dominiq at lps dot ens.fr @ 2013-11-28 23:04 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #3 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> After apllying this fix routine is inlined.

I have applied the following patch

--- ../_clean/gcc/ipa-inline.c    2013-11-22 17:27:28.000000000 +0100
+++ gcc/ipa-inline.c    2013-11-28 21:45:29.000000000 +0100
@@ -762,7 +762,7 @@ check_callers (struct cgraph_node *node,
      {
        if (!can_inline_edge_p (e, true))
          return true;
-       if (!has_hot_call && cgraph_maybe_hot_edge_p (e))
+       if (!(*(bool*)has_hot_call) && cgraph_maybe_hot_edge_p (e))
      *(bool *)has_hot_call = true;
      }
   return false;

on top of revision 205497. However I do not see any improvement.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (4 preceding siblings ...)
  2013-11-28 23:04 ` dominiq at lps dot ens.fr
@ 2013-12-02 12:12 ` ysrumyan at gmail dot com
  2013-12-02 12:13 ` ysrumyan at gmail dot com
                   ` (19 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: ysrumyan at gmail dot com @ 2013-12-02 12:12 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #4 from Yuri Rumyantsev <ysrumyan at gmail dot com> ---
It turned out that proposed fix does not help trunk compilers since now another
huge routine is inlined firstly (read_input) and for perdida we got the
following message:

not inlinable: iztaccihuatl/17 -> perdida.constprop/122, --param
large-function-growth limit reached

Note that passing --param large-function-growth=130 will restore performance.

I assume that additional heuristics must be designed to inline once called
functions which are called inside inner loop.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (5 preceding siblings ...)
  2013-12-02 12:12 ` ysrumyan at gmail dot com
@ 2013-12-02 12:13 ` ysrumyan at gmail dot com
  2013-12-03 15:02 ` dominiq at lps dot ens.fr
                   ` (18 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: ysrumyan at gmail dot com @ 2013-12-02 12:13 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #5 from Yuri Rumyantsev <ysrumyan at gmail dot com> ---
Created attachment 31348
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31348&action=edit
test-case to reproduce

It need to be compiled with -Ofast -flto options to reproduce.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (6 preceding siblings ...)
  2013-12-02 12:13 ` ysrumyan at gmail dot com
@ 2013-12-03 15:02 ` dominiq at lps dot ens.fr
  2013-12-03 15:55 ` ysrumyan at gmail dot com
                   ` (17 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: dominiq at lps dot ens.fr @ 2013-12-03 15:02 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #6 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> It turned out that proposed fix does not help trunk compilers 
> since now another huge routine is inlined firstly (read_input) 
> and for perdida we got the following message:

This has been seen before and supposed to be fixed: see pr48636, comment #37.

> Note that passing --param large-function-growth=130 will restore performance.

As does --param max-inline-insns-auto=94.

> I assume that additional heuristics must be designed to inline once 
> called functions which are called inside inner loop.

Well, it was supposed to have been found. What did break it?


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (7 preceding siblings ...)
  2013-12-03 15:02 ` dominiq at lps dot ens.fr
@ 2013-12-03 15:55 ` ysrumyan at gmail dot com
  2013-12-06  9:54 ` dominiq at lps dot ens.fr
                   ` (16 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: ysrumyan at gmail dot com @ 2013-12-03 15:55 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #7 from Yuri Rumyantsev <ysrumyan at gmail dot com> ---
I saw that on old compiler sources (dated by 20130911) with my patch 'perdida'
was inlined without any additional inline parameters (using -flto) but now it
does not inlined since another large function read_input is inlined before it
and we reach max growth limit. So I assume that an order of call graph walking
is different now. It means that we really need another heuristics to
distinguish really cold and hot calls, e.g. we can use edge->count for sorting
inline candidates.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (8 preceding siblings ...)
  2013-12-03 15:55 ` ysrumyan at gmail dot com
@ 2013-12-06  9:54 ` dominiq at lps dot ens.fr
  2013-12-06 15:19 ` hubicka at gcc dot gnu.org
                   ` (15 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: dominiq at lps dot ens.fr @ 2013-12-06  9:54 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #8 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> ... bug was introduced by r202567 ...

I have checked that and fixing it with r205586 restores the timing of r202566.
The reason why this is not seen on recent revisions is r203167 which introduces
yet another parameter 'builtin-expect-probability' with a default value 90.
Setting this parameter to 92 allows the run time for fatigue2 to go from 78.2s
to 29.1s (26.6s with r202566).

It does not look reasonable to control the inlining with at least three
parameters: 'large-function-growth', 'max-inline-insns-auto', and
'builtin-expect-probability'. In top of that their documentation is scarce.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (9 preceding siblings ...)
  2013-12-06  9:54 ` dominiq at lps dot ens.fr
@ 2013-12-06 15:19 ` hubicka at gcc dot gnu.org
  2013-12-06 16:25 ` hubicka at gcc dot gnu.org
                   ` (14 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: hubicka at gcc dot gnu.org @ 2013-12-06 15:19 UTC (permalink / raw)
  To: gcc-bugs

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |hubicka at gcc dot gnu.org

--- Comment #9 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I will look into it...


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (10 preceding siblings ...)
  2013-12-06 15:19 ` hubicka at gcc dot gnu.org
@ 2013-12-06 16:25 ` hubicka at gcc dot gnu.org
  2013-12-06 18:20 ` hubicka at gcc dot gnu.org
                   ` (13 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: hubicka at gcc dot gnu.org @ 2013-12-06 16:25 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #10 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
OK,
seems that the problem is with Fortran generating internally __builtin_expect
to control various construct.  I would make a lot more sense to use
GIMPLE_PREDICT for those cases. With GIMPLE_PREDICT one can have more fine
grained control over
the hint and handle it as separate predicter.

I wonder if some Fortran maintainer can look into this and update the lowering?
c/c-typeck.c:    add_stmt (build_predict_expr (PRED_CONTINUE, NOT_TAKEN));
cp/cp-gimplify.c:  tree pred = build_predict_expr (PRED_CONTINUE, NOT_TAKEN);

are examples how these are used.  Unlike builtin_expect you do not add it into
the expression, but you drop it on the unlikely code path.

Honza


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (11 preceding siblings ...)
  2013-12-06 16:25 ` hubicka at gcc dot gnu.org
@ 2013-12-06 18:20 ` hubicka at gcc dot gnu.org
  2013-12-06 19:18 ` dominiq at lps dot ens.fr
                   ` (12 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: hubicka at gcc dot gnu.org @ 2013-12-06 18:20 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #11 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
The inlining of perdida also happens with --param large-function-insns=100000.
perhaps it indicates we shoud bump this parameter up little bit.

The reason why inlining order changed is iztaccihuatl that calls perdida.
The function has 78 checks that leads to early exit (because of out of memory I
believe). Before the __builtin_expect change we predicted them all with 0.01%
probability to exit, while now we predict them with 0.1.

Obviously 0.9^78 is a lot less than 0.99^78.

Fortran fronend should probably stop using builtin_expect where we can derive
the same info from noreturn (as it seems to be the case here), but it is not
always.

I wonder if we want to add optional probability argument to builtlin_expect? 
It is not completely trvial addition since expr_expected_value_1 does some
propagation and it will need to be extended to handle the probablities, too.

Honza


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (12 preceding siblings ...)
  2013-12-06 18:20 ` hubicka at gcc dot gnu.org
@ 2013-12-06 19:18 ` dominiq at lps dot ens.fr
  2013-12-08 11:44 ` Joost.VandeVondele at mat dot ethz.ch
                   ` (11 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: dominiq at lps dot ens.fr @ 2013-12-06 19:18 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #12 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> The inlining of perdida also happens with --param large-function-insns=100000.
> perhaps it indicates we shoud bump this parameter up little bit.

The threshold is ~6000 (exactly 5941), i.e. more than twice the default value
2700.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (13 preceding siblings ...)
  2013-12-06 19:18 ` dominiq at lps dot ens.fr
@ 2013-12-08 11:44 ` Joost.VandeVondele at mat dot ethz.ch
  2013-12-10  9:20 ` hubicka at ucw dot cz
                   ` (10 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: Joost.VandeVondele at mat dot ethz.ch @ 2013-12-08 11:44 UTC (permalink / raw)
  To: gcc-bugs

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

Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Joost.VandeVondele at mat dot ethz
                   |                            |.ch

--- Comment #13 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Jan Hubicka from comment #11)
> Fortran fronend should probably stop using builtin_expect where we can
> derive the same info from noreturn (as it seems to be the case here), but it
> is not always.

Actually, I would argue that the middle-end should be smart enough to give a
branch that is guaranteed to never return a negligible probability (independent
of the builtin_expect). It can only be mis-predicted once.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (14 preceding siblings ...)
  2013-12-08 11:44 ` Joost.VandeVondele at mat dot ethz.ch
@ 2013-12-10  9:20 ` hubicka at ucw dot cz
  2013-12-10  9:23 ` hubicka at ucw dot cz
                   ` (9 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: hubicka at ucw dot cz @ 2013-12-10  9:20 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #14 from Jan Hubicka <hubicka at ucw dot cz> ---
> Actually, I would argue that the middle-end should be smart enough to give a
> branch that is guaranteed to never return a negligible probability (independent
> of the builtin_expect). It can only be mis-predicted once.

For predicting branches, we have gimple predict_stmt.
If we need to annotate values with higher probability, I will implement the
extension
into bulitin_expect to handle them.
(i.e. adding internal only second argument specifying the predictor)
Sounds sane?


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (15 preceding siblings ...)
  2013-12-10  9:20 ` hubicka at ucw dot cz
@ 2013-12-10  9:23 ` hubicka at ucw dot cz
  2013-12-10 22:30 ` burnus at gcc dot gnu.org
                   ` (8 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: hubicka at ucw dot cz @ 2013-12-10  9:23 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #15 from Jan Hubicka <hubicka at ucw dot cz> ---
> The threshold is ~6000 (exactly 5941), i.e. more than twice the default value
> 2700.
Thanks for working it out.  This may be only testcase I know of where
large-function-insns
brings significant regression.

Did you try to experiment with large-stack-frame limits for fortran?  Those hit
quite commonly, perhaps we miss useful inlining here.

I never did really serious tunning on large-function-insns except for quick
check that
increasing it doesn't affect SPEC scores.  But lets fix the underlying branch
prediction
problem and see if we find another testcase supporting the change.

Honza


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (16 preceding siblings ...)
  2013-12-10  9:23 ` hubicka at ucw dot cz
@ 2013-12-10 22:30 ` burnus at gcc dot gnu.org
  2013-12-26 15:51 ` hubicka at gcc dot gnu.org
                   ` (7 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: burnus at gcc dot gnu.org @ 2013-12-10 22:30 UTC (permalink / raw)
  To: gcc-bugs

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

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #16 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #14)
> For predicting branches, we have gimple predict_stmt.

Request for assistance/comment patch for gfortran:
http://gcc.gnu.org/ml/gcc-patches/2013-12/msg01034.html


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (17 preceding siblings ...)
  2013-12-10 22:30 ` burnus at gcc dot gnu.org
@ 2013-12-26 15:51 ` hubicka at gcc dot gnu.org
  2014-03-02  6:24 ` burnus at gcc dot gnu.org
                   ` (6 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: hubicka at gcc dot gnu.org @ 2013-12-26 15:51 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #17 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Created attachment 31517
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31517&action=edit
Patch to extend __builtin_expect

Hi,
this patch adds internal use only parameter to builtin_expect that Fortran can
use to specify type of in builtin_unlikely/likely.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (18 preceding siblings ...)
  2013-12-26 15:51 ` hubicka at gcc dot gnu.org
@ 2014-03-02  6:24 ` burnus at gcc dot gnu.org
  2014-03-14 14:53 ` jakub at gcc dot gnu.org
                   ` (5 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: burnus at gcc dot gnu.org @ 2014-03-02  6:24 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #18 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Tobias Burnus from comment #16)
> Request for assistance/comment patch for gfortran:
> http://gcc.gnu.org/ml/gcc-patches/2013-12/msg01034.html

(In reply to Jan Hubicka from comment #17)
> Created attachment 31517 [details] Patch to extend __builtin_expect

Updated RFC patch,based on the extend-__builtin_expect patch:
http://gcc.gnu.org/ml/gcc-patches/2014-03/msg00043.html


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (19 preceding siblings ...)
  2014-03-02  6:24 ` burnus at gcc dot gnu.org
@ 2014-03-14 14:53 ` jakub at gcc dot gnu.org
  2014-03-14 17:27 ` jakub at gcc dot gnu.org
                   ` (4 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: jakub at gcc dot gnu.org @ 2014-03-14 14:53 UTC (permalink / raw)
  To: gcc-bugs

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 32347
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32347&action=edit
gcc49-pr58721.patch

Untested patch.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (20 preceding siblings ...)
  2014-03-14 14:53 ` jakub at gcc dot gnu.org
@ 2014-03-14 17:27 ` jakub at gcc dot gnu.org
  2014-03-14 18:11 ` redi at gcc dot gnu.org
                   ` (3 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: jakub at gcc dot gnu.org @ 2014-03-14 17:27 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #20 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Unfortunately the patch regresses abi_check in libstdc++, libstdc++.so.6 now
because of the patch exports
_ZNSt12system_errorC1ESt10error_codeRKSs@@GLIBCXX_3.4.11
_ZNSt12system_errorC2ESt10error_codeRKSs@@GLIBCXX_3.4.11
symbols.
This is caused by wastly different inlining decisions (and the decisions don't
look like an improvement to me) on thread.cc.
Debugging this, I'm seeing this is caused by Honza's predict.c changes.
E.g. quite often we have tree_predict_by_opcode called on
&__gthrw___pthread_key_create != 0.  When we call expr_expected_value_1 on
this,
it returns a NE_EXPR (__gthrw___pthread_key_create is weakref), but with
TREE_CONSTANT set on it.  That is reasonable, the comparison will be either
always true, or always false, but without explicit __builtin_expect from the
user I actually don't see a reason why we should predict it as always true.

So, I wonder if we shouldn't do on top of the #c19 patch:
--- gcc/predict.c.jj    2014-03-14 13:16:15.000000000 +0100
+++ gcc/predict.c    2014-03-14 17:49:11.686411889 +0100
@@ -2066,7 +2066,7 @@ tree_predict_by_opcode (basic_block bb)
   visited = BITMAP_ALLOC (NULL);
   val = expr_expected_value_1 (boolean_type_node, op0, cmp, op1, visited,
&predictor);
   BITMAP_FREE (visited);
-  if (val && TREE_CONSTANT (val))
+  if (val && TREE_CODE (val) == INTEGER_CST)
     {
       if (predictor == PRED_BUILTIN_EXPECT)
     {
That change seems to slightly improve the inlining decisions on thread.ii, e.g.
_ZN9__gnu_cxxL27__exchange_and_add_dispatchEPii.part.1.constprop.11 seems to be
inlined again, but _ZNSt12system_errorC1ESt10error_codeRKSs and
_ZNSt12system_errorC2ESt10error_codeRKSs are still no longer inlined.
Comparing to gcc with all the patches but all the predict.c #c19 and the above
bits reverted, I see that (looking at -m32 -O2 ...) the
_ZNSt12system_errorC1ESt10error_codeRKSs function (the other symbol is an alias
to it) is 601 bytes long and
_ZNSt6thread15_M_start_threadESt10shared_ptrINS_10_Impl_baseEE caller
406 bytes long with the predict.c changes and
_ZNSt6thread15_M_start_threadESt10shared_ptrINS_10_Impl_baseEE is 889 bytes
long with the predict.c changes reverted (the
_ZNSt12system_errorC1ESt10error_codeRKSs function is inlined then).
So, size wise it is a win to inline it, but not significant, and the function
is in any case larger than the caller.

Honza, your thoughts on this?


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (21 preceding siblings ...)
  2014-03-14 17:27 ` jakub at gcc dot gnu.org
@ 2014-03-14 18:11 ` redi at gcc dot gnu.org
  2014-03-14 18:12 ` redi at gcc dot gnu.org
                   ` (2 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: redi at gcc dot gnu.org @ 2014-03-14 18:11 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #21 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Author: redi
Date: Fri Mar 14 18:10:59 2014
New Revision: 208572

URL: http://gcc.gnu.org/viewcvs?rev=208572&root=gcc&view=rev
Log:
    PR ipa/58721
    * config/abi/pre/gnu.ver (GLIBCXX_3.4.11): Remove unused pattern for
    _ZNSt12system_errorC* symbols which are not exported on any target.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/config/abi/pre/gnu.ver


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (22 preceding siblings ...)
  2014-03-14 18:11 ` redi at gcc dot gnu.org
@ 2014-03-14 18:12 ` redi at gcc dot gnu.org
  2014-03-18 11:31 ` jakub at gcc dot gnu.org
  2014-03-18 11:39 ` jakub at gcc dot gnu.org
  25 siblings, 0 replies; 27+ messages in thread
From: redi at gcc dot gnu.org @ 2014-03-14 18:12 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #22 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #20)
> Unfortunately the patch regresses abi_check in libstdc++, libstdc++.so.6 now
> because of the patch exports
> _ZNSt12system_errorC1ESt10error_codeRKSs@@GLIBCXX_3.4.11
> _ZNSt12system_errorC2ESt10error_codeRKSs@@GLIBCXX_3.4.11
> symbols.

Those will not be exported now.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (23 preceding siblings ...)
  2014-03-14 18:12 ` redi at gcc dot gnu.org
@ 2014-03-18 11:31 ` jakub at gcc dot gnu.org
  2014-03-18 11:39 ` jakub at gcc dot gnu.org
  25 siblings, 0 replies; 27+ messages in thread
From: jakub at gcc dot gnu.org @ 2014-03-18 11:31 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #23 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Tue Mar 18 11:31:04 2014
New Revision: 208641

URL: http://gcc.gnu.org/viewcvs?rev=208641&root=gcc&view=rev
Log:
    PR ipa/58721
gcc/
    * internal-fn.c: Include diagnostic-core.h.
    (expand_BUILTIN_EXPECT): New function.
    * gimplify.c (gimplify_call_expr): Use false instead of FALSE.
    (gimplify_modify_expr): Gimplify 3 argument __builtin_expect into
    IFN_BUILTIN_EXPECT call instead of __builtin_expect builtin call.
    * ipa-inline-analysis.c (find_foldable_builtin_expect): Handle
    IFN_BUILTIN_EXPECT.
    * predict.c (expr_expected_value_1): Handle IFN_BUILTIN_EXPECT.
    Revert 3 argument __builtin_expect code.
    (strip_predict_hints): Handle IFN_BUILTIN_EXPECT.
    * gimple-fold.c (gimple_fold_call): Likewise.
    * tree.h (fold_builtin_expect): New prototype.
    * builtins.c (build_builtin_expect_predicate): Add predictor
    argument, if non-NULL, create 3 argument __builtin_expect.
    (fold_builtin_expect): No longer static.  Add ARG2 argument,
    pass it through to build_builtin_expect_predicate.
    (fold_builtin_2): Adjust caller.
    (fold_builtin_3): Handle BUILT_IN_EXPECT.
    * internal-fn.def (BUILTIN_EXPECT): New.
gcc/fortran/
    * trans.c (gfc_unlikely, gfc_likely): Don't add __builtin_expect
    if !optimize.

2014-03-18  Tobias Burnus  <burnus@net-b.de>

    PR ipa/58721
gcc/
    * predict.def (PRED_FORTRAN_OVERFLOW, PRED_FORTRAN_FAIL_ALLOC,
    PRED_FORTRAN_FAIL_IO, PRED_FORTRAN_WARN_ONCE, PRED_FORTRAN_SIZE_ZERO,
    PRED_FORTRAN_INVALID_BOUND, PRED_FORTRAN_ABSENT_DUMMY): Add.
gcc/fortran/
    * trans.h (gfc_unlikely, gfc_likely): Add predictor as argument.
    (gfc_trans_io_runtime_check): Remove.
    * trans-io.c (gfc_trans_io_runtime_check): Make static; add has_iostat
    as argument, add predictor to block.
    (set_parameter_value, gfc_trans_open, gfc_trans_close, build_filepos,
    gfc_trans_inquire, gfc_trans_wait, build_dt): Update calls.
    * trans.c (gfc_unlikely, gfc_likely): Add predictor as argument.
    (gfc_trans_runtime_check, gfc_allocate_using_malloc,
    gfc_allocate_allocatable, gfc_deallocate_with_status): Set explicitly
    branch predictor.
    * trans-expr.c (gfc_conv_procedure_call): Ditto.
    * trans-stmt.c (gfc_trans_allocate): Ditto.
    * trans-array.c (gfc_array_init_size, gfc_array_allocate): Ditto.

2014-03-18  Jan Hubicka  <hubicka@ucw.cz>

    PR ipa/58721
gcc/
    * predict.c (combine_predictions_for_bb): Fix up formatting.
    (expr_expected_value_1, expr_expected_value): Add predictor argument,
    fill what it points to if non-NULL.
    (tree_predict_by_opcode): Adjust caller, use the predictor.
    * predict.def (PRED_COMPARE_AND_SWAP): Add.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/builtins.c
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/fortran/trans-intrinsic.c
    trunk/gcc/fortran/trans-io.c
    trunk/gcc/fortran/trans-stmt.c
    trunk/gcc/fortran/trans.c
    trunk/gcc/fortran/trans.h
    trunk/gcc/gimple-fold.c
    trunk/gcc/gimplify.c
    trunk/gcc/internal-fn.c
    trunk/gcc/internal-fn.def
    trunk/gcc/ipa-inline-analysis.c
    trunk/gcc/predict.c
    trunk/gcc/predict.def
    trunk/gcc/tree.h


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug ipa/58721] [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90
  2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
                   ` (24 preceding siblings ...)
  2014-03-18 11:31 ` jakub at gcc dot gnu.org
@ 2014-03-18 11:39 ` jakub at gcc dot gnu.org
  25 siblings, 0 replies; 27+ messages in thread
From: jakub at gcc dot gnu.org @ 2014-03-18 11:39 UTC (permalink / raw)
  To: gcc-bugs

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #24 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Hopefully fixed.


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2014-03-18 11:39 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-14  8:22 [Bug tree-optimization/58721] New: [4.9 Regression] The subroutine perdida is no longer inlined in fatigue.f90 dominiq at lps dot ens.fr
2013-10-14  8:59 ` [Bug ipa/58721] " rguenth at gcc dot gnu.org
2013-11-05 15:02 ` rguenth at gcc dot gnu.org
2013-11-28 14:01 ` ysrumyan at gmail dot com
2013-11-28 19:19 ` hjl.tools at gmail dot com
2013-11-28 23:04 ` dominiq at lps dot ens.fr
2013-12-02 12:12 ` ysrumyan at gmail dot com
2013-12-02 12:13 ` ysrumyan at gmail dot com
2013-12-03 15:02 ` dominiq at lps dot ens.fr
2013-12-03 15:55 ` ysrumyan at gmail dot com
2013-12-06  9:54 ` dominiq at lps dot ens.fr
2013-12-06 15:19 ` hubicka at gcc dot gnu.org
2013-12-06 16:25 ` hubicka at gcc dot gnu.org
2013-12-06 18:20 ` hubicka at gcc dot gnu.org
2013-12-06 19:18 ` dominiq at lps dot ens.fr
2013-12-08 11:44 ` Joost.VandeVondele at mat dot ethz.ch
2013-12-10  9:20 ` hubicka at ucw dot cz
2013-12-10  9:23 ` hubicka at ucw dot cz
2013-12-10 22:30 ` burnus at gcc dot gnu.org
2013-12-26 15:51 ` hubicka at gcc dot gnu.org
2014-03-02  6:24 ` burnus at gcc dot gnu.org
2014-03-14 14:53 ` jakub at gcc dot gnu.org
2014-03-14 17:27 ` jakub at gcc dot gnu.org
2014-03-14 18:11 ` redi at gcc dot gnu.org
2014-03-14 18:12 ` redi at gcc dot gnu.org
2014-03-18 11:31 ` jakub at gcc dot gnu.org
2014-03-18 11:39 ` jakub at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).