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
