[Bug tree-optimization/63970] New: gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[Bug tree-optimization/63970] New: gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[wmi at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

            Bug ID: 63970
           Summary: gcc-4_9 inlines less funcs than gcc-4_8 because of
                    used_as_abstract_origin flag
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: wmi at google dot com
                CC: davidxl at google dot com, dehao at google dot com,
                    hubicka at gcc dot gnu.org, rguenth at gcc dot gnu.org,
                    tejohnson at google dot com

We see an inline problem as below caused by r201408
(https://gcc.gnu.org/ml/gcc-patches/2013-08/msg00027.html).

hoo() {
  foo();
  ...
}

foo {
  goo();
  ...
}

foo is func splitted, so its body changes to

foo {
  goo();
  ...
  foo.part();
}

and the used_as_abstract_origin of cgraph node of foo will be set to
true after func splitting.

In ipa-inline, when inlining foo into hoo, the original node of foo
will not be reused as clone node because used_as_abstract_origin of
cgraph node of foo is true and can_remove_node_now_p_1 will return
false, so that a new clone node of foo will be created. This is the
case in gcc-4_9.
In gcc-4_8, the original node of foo will be reused as clone node.

gcc-4_8
foo
  |
goo

gcc-4_9
foo        foo_clone
    \     /
      goo

Because of the difference of whether to create a new clone for foo,
when inlining goo to foo, the overall growth of inlining all callsites
of goo in gcc-4_8 will be less than gcc-4_9 (goo has two callsites in
gcc-4_9 but only one in gcc-4_8). If we have many cases like this,
gcc-4_8 will actually have more inline growth budget than gcc-4_9 and
will inline more aggressively than gcc-4_9.

I don't understand the exact usage of the check about
node->used_as_abstract_origin in can_remove_node_now_p_1, but I feel
puzzled about following two points:

1. https://gcc.gnu.org/ml/gcc-patches/2013-08/msg00027.html said the
patch was to ensure all abstract origin functions do have nodes
attached. However, even if the node of origin function is reused as a
clone node, a new clone node will be created in following code in
symbol_table::remove_unreachable_nodes if only the node that needs
abstract origin is reachable.

          if (TREE_CODE (node->decl) == FUNCTION_DECL
              && DECL_ABSTRACT_ORIGIN (node->decl))
            {
              struct cgraph_node *origin_node
              = cgraph_node::get_create (DECL_ABSTRACT_ORIGIN (node->decl));
              origin_node->used_as_abstract_origin = true;
              enqueue_node (origin_node, &first, &reachable);
            }

2. DECL_ABSTRACT_ORIGIN(decl) seems only useful for debug info of
clone nodes. But now the check of used_as_abstract_origin affect
inline decisions, which should be the same with or without keeping
debug info.

[Bug tree-optimization/63970] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[wmi at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

--- Comment #1 from wmi at google dot com ---
> I think we need to keep the functions but do not need to account for them in the unit size if we otherwise could remove them
>
> Richard.

But there is code in symbol_table::remove_unreachable_nodes:

          if (TREE_CODE (node->decl) == FUNCTION_DECL
              && DECL_ABSTRACT_ORIGIN (node->decl))
            {
              struct cgraph_node *origin_node
              = cgraph_node::get_create (DECL_ABSTRACT_ORIGIN (node->decl));
              origin_node->used_as_abstract_origin = true;
              enqueue_node (origin_node, &first, &reachable);
            }

If we remove the check in can_remove_node_now_p_1, the original node will be
removed or reused as clone node in ipa inline analysis, but it will be
recreated in symbol_table::remove_unreachable_nodes after ipa inline analysis
finishes, if only its clone nodes are reachable.

So can we just remove the original node in inline analysis and let
symbol_table::remove_unreachable_nodes to restore it after ipa inline analysis?

Thanks,
Wei.

[Bug tree-optimization/63970] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

--- Comment #2 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I do not think we have problem with -g/without, because abstract origins are
set independently on debugger setting.

Normally callgraph will release function body when it is found unreachable.
This breaks debug info if the function appears as abstract origin, because it
prevents dwarf2out from being able to output its abstract DIE. The flag simply
prevents releasing such function bodies.

I suppose we can drop the test in can_remove_node_now_p_1 but we need to ensure
that all inline clones actually gets the flag set (because we do not know which
one will be last removed from callgraph and triggering the elimination).
This is probably easy to do:
 - clone_inlined_nodes needs to copy the used_as_abstract_origin flag from its
   master
 - symbol_table::remove_unreachable_nodes needs to walk all clones and set
their flag.

[Bug tree-optimization/63970] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

--- Comment #3 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Created attachment 34047
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34047&action=edit
Patch

Something like this (untested) may work

[Bug ipa/63970] [4.9/5 Regression] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
          Component|tree-optimization           |ipa
   Target Milestone|---                         |4.9.3
            Summary|gcc-4_9 inlines less funcs  |[4.9/5 Regression] gcc-4_9
                   |than gcc-4_8 because of     |inlines less funcs than
                   |used_as_abstract_origin     |gcc-4_8 because of
                   |flag                        |used_as_abstract_origin
                   |                            |flag

[Bug ipa/63970] [4.9/5 Regression] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[wmi at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

--- Comment #4 from wmi at google dot com ---
(In reply to Jan Hubicka from comment #3)
> Created attachment 34047 [details]
> Patch
> 
> Something like this (untested) may work

Thanks! I tested your patch after minor change. It passed bootstrap and
regression. It also solved the performance regression we saw in internal
benchmarks. 

+          if (origin_node && !origin_node->used_as_abstract_origin)
+        {
+              origin_node->used_as_abstract_origin = true;
+          enqueue_node (origin_node, &first, &reachable); // enqueue_node
moved here
+          gcc_assert (!origin_node->prev_sibling_clone);
+          gcc_assert (!origin_node->next_sibling_clone);
+          for (origin_node = origin_node->clones; origin_node;
+               origin_node = origin_node->next_sibling_clone)
+            if (origin_node->decl == DECL_ABSTRACT_ORIGIN (node->decl))
+              origin_node->used_as_abstract_origin = true;
+        }

Wei.

[Bug ipa/63970] [4.9/5 Regression] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2014-11-24
     Ever confirmed|0                           |1

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
I believe sth has been committed - please update the bug.  (and remember to
refer to PRs in changelogs)

[Bug ipa/63970] [4.9/5 Regression] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[wmi at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

--- Comment #6 from wmi at google dot com ---
The patch was committed to trunk at r217973.

[Bug ipa/63970] [4.9 Regression] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
             Status|WAITING                     |NEW
      Known to work|                            |5.0
            Summary|[4.9/5 Regression] gcc-4_9  |[4.9 Regression] gcc-4_9
                   |inlines less funcs than     |inlines less funcs than
                   |gcc-4_8 because of          |gcc-4_8 because of
                   |used_as_abstract_origin     |used_as_abstract_origin
                   |flag                        |flag

[Bug ipa/63970] [4.9 Regression] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[wmi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

--- Comment #7 from wmi at gcc dot gnu.org ---
Author: wmi
Date: Wed Jan 21 21:56:14 2015
New Revision: 219972

URL: https://gcc.gnu.org/viewcvs?rev=219972&root=gcc&view=rev
Log:
        Backported from trunk.
        2014-11-22  Jan Hubicka  <hubicka@ucw.cz>

        PR ipa/63970
        * ipa.c (symbol_table::remove_unreachable_nodes): Mark all inline
clones
        as having abstract origin used.
        * ipa-inline-transform.c (can_remove_node_now_p_1): Drop abstract
origin check.
        (clone_inlined_nodes): Copy abstract originflag.
        * lto-cgraph.c (compute_ltrans_boundary): Use get_create to get
abstract origin node.

Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/ipa-inline-transform.c
    branches/gcc-4_9-branch/gcc/ipa.c
    branches/gcc-4_9-branch/gcc/lto-cgraph.c

[Bug ipa/63970] [4.9 Regression] gcc-4_9 inlines less funcs than gcc-4_8 because of used_as_abstract_origin flag

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63970

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
      Known to work|                            |4.9.3
         Resolution|---                         |FIXED
      Known to fail|                            |4.9.2

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.