diff mbox series

AutoFDO: Don't try to promote indirect calls that result in recursive direct calls

Message ID MW2PR2101MB11009C56322649EE61083026912E9@MW2PR2101MB1100.namprd21.prod.outlook.com
State New
Headers
Series AutoFDO: Don't try to promote indirect calls that result in recursive direct calls |

Commit Message

Eugene Rozenfeld Feb. 9, 2022, 7:15 a.m. UTC 
  AutoFDO tries to promote and inline all indirect calls that were promoted
and inlined in the original binary and that are still hot. In the included
test case, the promotion results in a direct call that is a recursive call.
inline_call and optimize_inline_calls can't handle recursive calls at this stage.
Currently, inline_call fails with a segmentation fault.

This change leaves the indirect call alone if promotion will result in a recursive call.

Tested on x86_64-pc-linux-gnu.

gcc/ChangeLog:
        * auto-profile.cc (afdo_indirect_call): Don't attempt to promote indirect calls
        that will result in direct recursive calls.

gcc/testsuite/g++.dg/tree-prof/ChangeLog:
        * indir-call-recursive-inlining.C : New test.
---
 gcc/auto-profile.cc                           | 40 ++++++++------
 .../tree-prof/indir-call-recursive-inlining.C | 54 +++++++++++++++++++
 2 files changed, 78 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C

--
2.25.1

Comments

Richard Biener Feb. 9, 2022, 8:35 a.m. UTC | #1 
On Wed, Feb 9, 2022 at 8:16 AM Eugene Rozenfeld via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> AutoFDO tries to promote and inline all indirect calls that were promoted
> and inlined in the original binary and that are still hot. In the included
> test case, the promotion results in a direct call that is a recursive call.
> inline_call and optimize_inline_calls can't handle recursive calls at this stage.
> Currently, inline_call fails with a segmentation fault.
>
> This change leaves the indirect call alone if promotion will result in a recursive call.
>
> Tested on x86_64-pc-linux-gnu.

OK.

Thanks,
Richard.

> gcc/ChangeLog:
>         * auto-profile.cc (afdo_indirect_call): Don't attempt to promote indirect calls
>         that will result in direct recursive calls.
>
> gcc/testsuite/g++.dg/tree-prof/ChangeLog:
>         * indir-call-recursive-inlining.C : New test.
> ---
>  gcc/auto-profile.cc                           | 40 ++++++++------
>  .../tree-prof/indir-call-recursive-inlining.C | 54 +++++++++++++++++++
>  2 files changed, 78 insertions(+), 16 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C
>
> diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
> index c7cee639c85..2b34b80b82d 100644
> --- a/gcc/auto-profile.cc
> +++ b/gcc/auto-profile.cc
> @@ -975,7 +975,7 @@ read_profile (void)
>       * after annotation, we just need to mark, and let follow-up logic to
>         decide if it needs to promote and inline.  */
>
> -static void
> +static bool
>  afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
>                      bool transform)
>  {
> @@ -983,12 +983,12 @@ afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
>    tree callee;
>
>    if (map.size () == 0)
> -    return;
> +    return false;
>    gcall *stmt = dyn_cast <gcall *> (gs);
>    if (!stmt
>        || gimple_call_internal_p (stmt)
>        || gimple_call_fndecl (stmt) != NULL_TREE)
> -    return;
> +    return false;
>
>    gcov_type total = 0;
>    icall_target_map::const_iterator max_iter = map.end ();
> @@ -1003,7 +1003,7 @@ afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
>    struct cgraph_node *direct_call = cgraph_node::get_for_asmname (
>        get_identifier (afdo_string_table->get_name (max_iter->first)));
>    if (direct_call == NULL || !direct_call->profile_id)
> -    return;
> +    return false;
>
>    callee = gimple_call_fn (stmt);
>
> @@ -1013,20 +1013,27 @@ afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
>    hist->hvalue.counters = XNEWVEC (gcov_type, hist->n_counters);
>    gimple_add_histogram_value (cfun, stmt, hist);
>
> -  // Total counter
> +  /* Total counter */
>    hist->hvalue.counters[0] = total;
> -  // Number of value/counter pairs
> +  /* Number of value/counter pairs */
>    hist->hvalue.counters[1] = 1;
> -  // Value
> +  /* Value */
>    hist->hvalue.counters[2] = direct_call->profile_id;
> -  // Counter
> +  /* Counter */
>    hist->hvalue.counters[3] = max_iter->second;
>
>    if (!transform)
> -    return;
> +    return false;
> +
> +  cgraph_node* current_function_node = cgraph_node::get (current_function_decl);
> +
> +  /* If the direct call is a recursive call, don't promote it since
> +     we are not set up to inline recursive calls at this stage. */
> +  if (direct_call == current_function_node)
> +    return false;
>
>    struct cgraph_edge *indirect_edge
> -      = cgraph_node::get (current_function_decl)->get_edge (stmt);
> +      = current_function_node->get_edge (stmt);
>
>    if (dump_file)
>      {
> @@ -1040,13 +1047,13 @@ afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
>      {
>        if (dump_file)
>          fprintf (dump_file, " not transforming\n");
> -      return;
> +      return false;
>      }
>    if (DECL_STRUCT_FUNCTION (direct_call->decl) == NULL)
>      {
>        if (dump_file)
>          fprintf (dump_file, " no declaration\n");
> -      return;
> +      return false;
>      }
>
>    if (dump_file)
> @@ -1063,16 +1070,17 @@ afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
>    cgraph_edge::redirect_call_stmt_to_callee (new_edge);
>    gimple_remove_histogram_value (cfun, stmt, hist);
>    inline_call (new_edge, true, NULL, NULL, false);
> +  return true;
>  }
>
>  /* From AutoFDO profiles, find values inside STMT for that we want to measure
>     histograms and adds them to list VALUES.  */
>
> -static void
> +static bool
>  afdo_vpt (gimple_stmt_iterator *gsi, const icall_target_map &map,
>            bool transform)
>  {
> -  afdo_indirect_call (gsi, map, transform);
> +  return afdo_indirect_call (gsi, map, transform);
>  }
>
>  typedef std::set<basic_block> bb_set;
> @@ -1498,8 +1506,8 @@ afdo_vpt_for_early_inline (stmt_set *promoted_stmts)
>            {
>              /* Promote the indirect call and update the promoted_stmts.  */
>              promoted_stmts->insert (stmt);
> -            afdo_vpt (&gsi, info.targets, true);
> -            has_vpt = true;
> +            if (afdo_vpt (&gsi, info.targets, true))
> +              has_vpt = true;
>            }
>        }
>    }
> diff --git a/gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C b/gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C
> new file mode 100644
> index 00000000000..11f690063ef
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C
> @@ -0,0 +1,54 @@
> +/* { dg-options "-O2 " } */
> +
> +class Parent
> +{
> +public:
> +  Parent *object;
> +
> +  Parent()
> +  {
> +       object = this;
> +  }
> +
> +  virtual void recurse (int t) = 0;
> +};
> +
> +class Child : public Parent
> +{
> +
> +  Parent *
> +  get_object ()
> +  {
> +     return this;
> +  }
> +
> +public:
> +  virtual void
> +  recurse (int t)
> +  {
> +    if (t != 10)
> +      for (int i = 0; i < 5; ++i)
> +        get_object()->recurse(t + 1);
> +  };
> +};
> +
> +Parent *
> +create_object ()
> +{
> +  Child *mod = new Child;
> +  return mod;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  Parent *parent = create_object ();
> +
> +  for (int i = 0; i < 5; ++i)
> +    {
> +         parent->recurse (0);
> +    }
> +
> +  return 0;
> +}
> +
> --
> 2.25.1
diff mbox series

Patch

diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
index c7cee639c85..2b34b80b82d 100644
--- a/gcc/auto-profile.cc
+++ b/gcc/auto-profile.cc
@@ -975,7 +975,7 @@  read_profile (void)
      * after annotation, we just need to mark, and let follow-up logic to
        decide if it needs to promote and inline.  */

-static void
+static bool
 afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
                     bool transform)
 {
@@ -983,12 +983,12 @@  afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
   tree callee;

   if (map.size () == 0)
-    return;
+    return false;
   gcall *stmt = dyn_cast <gcall *> (gs);
   if (!stmt
       || gimple_call_internal_p (stmt)
       || gimple_call_fndecl (stmt) != NULL_TREE)
-    return;
+    return false;

   gcov_type total = 0;
   icall_target_map::const_iterator max_iter = map.end ();
@@ -1003,7 +1003,7 @@  afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
   struct cgraph_node *direct_call = cgraph_node::get_for_asmname (
       get_identifier (afdo_string_table->get_name (max_iter->first)));
   if (direct_call == NULL || !direct_call->profile_id)
-    return;
+    return false;

   callee = gimple_call_fn (stmt);

@@ -1013,20 +1013,27 @@  afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
   hist->hvalue.counters = XNEWVEC (gcov_type, hist->n_counters);
   gimple_add_histogram_value (cfun, stmt, hist);

-  // Total counter
+  /* Total counter */
   hist->hvalue.counters[0] = total;
-  // Number of value/counter pairs
+  /* Number of value/counter pairs */
   hist->hvalue.counters[1] = 1;
-  // Value
+  /* Value */
   hist->hvalue.counters[2] = direct_call->profile_id;
-  // Counter
+  /* Counter */
   hist->hvalue.counters[3] = max_iter->second;

   if (!transform)
-    return;
+    return false;
+
+  cgraph_node* current_function_node = cgraph_node::get (current_function_decl);
+
+  /* If the direct call is a recursive call, don't promote it since
+     we are not set up to inline recursive calls at this stage. */
+  if (direct_call == current_function_node)
+    return false;

   struct cgraph_edge *indirect_edge
-      = cgraph_node::get (current_function_decl)->get_edge (stmt);
+      = current_function_node->get_edge (stmt);

   if (dump_file)
     {
@@ -1040,13 +1047,13 @@  afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
     {
       if (dump_file)
         fprintf (dump_file, " not transforming\n");
-      return;
+      return false;
     }
   if (DECL_STRUCT_FUNCTION (direct_call->decl) == NULL)
     {
       if (dump_file)
         fprintf (dump_file, " no declaration\n");
-      return;
+      return false;
     }

   if (dump_file)
@@ -1063,16 +1070,17 @@  afdo_indirect_call (gimple_stmt_iterator *gsi, const icall_target_map &map,
   cgraph_edge::redirect_call_stmt_to_callee (new_edge);
   gimple_remove_histogram_value (cfun, stmt, hist);
   inline_call (new_edge, true, NULL, NULL, false);
+  return true;
 }

 /* From AutoFDO profiles, find values inside STMT for that we want to measure
    histograms and adds them to list VALUES.  */

-static void
+static bool
 afdo_vpt (gimple_stmt_iterator *gsi, const icall_target_map &map,
           bool transform)
 {
-  afdo_indirect_call (gsi, map, transform);
+  return afdo_indirect_call (gsi, map, transform);
 }

 typedef std::set<basic_block> bb_set;
@@ -1498,8 +1506,8 @@  afdo_vpt_for_early_inline (stmt_set *promoted_stmts)
           {
             /* Promote the indirect call and update the promoted_stmts.  */
             promoted_stmts->insert (stmt);
-            afdo_vpt (&gsi, info.targets, true);
-            has_vpt = true;
+            if (afdo_vpt (&gsi, info.targets, true))
+              has_vpt = true;
           }
       }
   }
diff --git a/gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C b/gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C
new file mode 100644
index 00000000000..11f690063ef
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-prof/indir-call-recursive-inlining.C
@@ -0,0 +1,54 @@ 
+/* { dg-options "-O2 " } */
+
+class Parent
+{
+public:
+  Parent *object;
+
+  Parent()
+  {
+       object = this;
+  }
+
+  virtual void recurse (int t) = 0;
+};
+
+class Child : public Parent
+{
+
+  Parent *
+  get_object ()
+  {
+     return this;
+  }
+
+public:
+  virtual void
+  recurse (int t)
+  {
+    if (t != 10)
+      for (int i = 0; i < 5; ++i)
+        get_object()->recurse(t + 1);
+  };
+};
+
+Parent *
+create_object ()
+{
+  Child *mod = new Child;
+  return mod;
+}
+
+int
+main (int argc, char **argv)
+{
+  Parent *parent = create_object ();
+
+  for (int i = 0; i < 5; ++i)
+    {
+         parent->recurse (0);
+    }
+
+  return 0;
+}
+