Re: [PATCH, PR 45572] Fix two minor issues with indirect inlining

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Sep 22, 2010 at 11:51 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> PR 45572 has three testcases which exhibit two different bugs:
>
> 1) ipa_make_edge_direct_to_target can inadvertently lazily create new
> cgraph nodes when looking up a node for a decl with cgraph_node
> function. ?This node then has an uid which is larger than size of
> ipa_node_params_vector, causing index out of bounds assert. ?The
> function should not create new nodes and so I replaced the call with a
> call to cgraph_get_node.

That part sounds obvious, you might want to install it separately.

> 2) If recursive indirect edges are discovered during recursive
> inlining, they are added to new_edges but
> cgraph_decide_recursive_inlining then looks them itself and processes
> them immediately. ?When edges from new_edges are processed, it
> attempts to inline recursively them again which leads to all sorts of
> weird consequences. ?Fixed by teaching the indirect inlining machinery
> not to add recursive edges when doing recursive inlining, which is
> flagged by new parameters of ipa_propagate_indirect_call_infos and
> unfortunately also of cgraph_mark_inline_edge.

Can you expand on the "weird consequences"?  Could we instead
disable indirect inlining during recursive inlining (and would that
make the patch any prettier?).

Thanks,
Richard.

> Bootstrapped and tested on x86_64-linux, OK for trunk?
>
> Thanks,
>
> Martin
>
>
>
> 2010-09-21 ?Martin Jambor ?<mjambor@suse.cz>
>
> ? ? ? ?PR tree-optimization/45572
> ? ? ? ?* ipa-prop.c (ipa_make_edge_direct_to_target): Use cgraph_get_node
> ? ? ? ?instead of cgraph_node.
> ? ? ? ?(update_indirect_edges_after_inlining): New parameter ignore.
> ? ? ? ?(propagate_info_to_inlined_callees): Likewise.
> ? ? ? ?(ipa_propagate_indirect_call_infos): New parameter new_recursive.
> ? ? ? ?* ipa-inline.c (cgraph_mark_inline_edge): New parameter new_recursive.
> ? ? ? ?Updated all callers.
>
> ? ? ? ?* testsuite/g++.dg/torture/pr45572-1.C: New test.
> ? ? ? ?* testsuite/g++.dg/torture/pr45572-2.C: Likewise.
>
> Index: icln/gcc/ipa-inline.c
> ===================================================================
> --- icln.orig/gcc/ipa-inline.c
> +++ icln/gcc/ipa-inline.c
> @@ -305,11 +305,13 @@ cgraph_clone_inlined_nodes (struct cgrap
> ?/* Mark edge E as inlined and update callgraph accordingly. ?UPDATE_ORIGINAL
> ? ?specify whether profile of original function should be updated. ?If any new
> ? ?indirect edges are discovered in the process, add them to NEW_EDGES, unless
> - ? it is NULL. ?Return true iff any new callgraph edges were discovered as a
> - ? result of inlining. ?*/
> + ? it is NULL. ?NEW_RECURSIVE should be set to false iff new recursive edges
> + ? should not be added to NEW_EDGES. ?Return true iff any new callgraph edges
> + ? were discovered as a result of inlining. ?*/
>
> ?static bool
> ?cgraph_mark_inline_edge (struct cgraph_edge *e, bool update_original,
> + ? ? ? ? ? ? ? ? ? ? ? ?bool new_recursive,
> ? ? ? ? ? ? ? ? ? ? ? ? VEC (cgraph_edge_p, heap) **new_edges)
> ?{
> ? int old_size = 0, new_size = 0;
> @@ -343,7 +345,7 @@ cgraph_mark_inline_edge (struct cgraph_e
> ? /* FIXME: We should remove the optimize check after we ensure we never run
> ? ? ?IPA passes when not optimizng. ?*/
> ? if (flag_indirect_inlining && optimize)
> - ? ?return ipa_propagate_indirect_call_infos (curr, new_edges);
> + ? ?return ipa_propagate_indirect_call_infos (curr, new_recursive, new_edges);
> ? else
> ? ? return false;
> ?}
> @@ -365,7 +367,7 @@ cgraph_mark_inline (struct cgraph_edge *
> ? ? ? next = e->next_caller;
> ? ? ? if (e->caller == to && e->inline_failed)
> ? ? ? ?{
> - ? ? ? ? ?cgraph_mark_inline_edge (e, true, NULL);
> + ? ? ? ? ?cgraph_mark_inline_edge (e, true, false, NULL);
> ? ? ? ? ?if (e == edge)
> ? ? ? ? ? ?edge = next;
> ? ? ? ?}
> @@ -960,7 +962,7 @@ cgraph_decide_recursive_inlining (struct
> ? ? ? ? ?fprintf (dump_file, "\n");
> ? ? ? ?}
> ? ? ? cgraph_redirect_edge_callee (curr, master_clone);
> - ? ? ?cgraph_mark_inline_edge (curr, false, new_edges);
> + ? ? ?cgraph_mark_inline_edge (curr, false, false, new_edges);
> ? ? ? lookup_recursive_calls (node, curr->callee, heap);
> ? ? ? n++;
> ? ? }
> @@ -1245,7 +1247,7 @@ cgraph_decide_inlining_of_small_function
> ? ? ? ? ? ?}
> ? ? ? ? ?callee = edge->callee;
> ? ? ? ? ?gcc_checking_assert (!callee->global.inlined_to);
> - ? ? ? ? cgraph_mark_inline_edge (edge, true, &new_indirect_edges);
> + ? ? ? ? cgraph_mark_inline_edge (edge, true, true, &new_indirect_edges);
> ? ? ? ? ?if (flag_indirect_inlining)
> ? ? ? ? ? ?add_new_edges_to_heap (heap, new_indirect_edges);
>
> @@ -1409,7 +1411,7 @@ cgraph_flatten (struct cgraph_node *node
> ? ? ? ? ? ? ? ? cgraph_node_name (e->callee),
> ? ? ? ? ? ? ? ? cgraph_node_name (e->caller));
> ? ? ? orig_callee = e->callee;
> - ? ? ?cgraph_mark_inline_edge (e, true, NULL);
> + ? ? ?cgraph_mark_inline_edge (e, true, false, NULL);
> ? ? ? if (e->callee != orig_callee)
> ? ? ? ?orig_callee->aux = (void *)(size_t) INLINE_ALL;
> ? ? ? cgraph_flatten (e->callee);
> Index: icln/gcc/ipa-prop.c
> ===================================================================
> --- icln.orig/gcc/ipa-prop.c
> +++ icln/gcc/ipa-prop.c
> @@ -1444,7 +1444,7 @@ ipa_make_edge_direct_to_target (struct c
> ? target = TREE_OPERAND (target, 0);
> ? if (TREE_CODE (target) != FUNCTION_DECL)
> ? ? return NULL;
> - ?callee = cgraph_node (target);
> + ?callee = cgraph_get_node (target);
> ? if (!callee)
> ? ? return NULL;
>
> @@ -1543,6 +1543,7 @@ try_make_edge_direct_virtual_call (struc
> ?static bool
> ?update_indirect_edges_after_inlining (struct cgraph_edge *cs,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cgraph_node *node,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct cgraph_node *ignore,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?VEC (cgraph_edge_p, heap) **new_edges)
> ?{
> ? struct ipa_edge_args *top;
> @@ -1594,7 +1595,7 @@ update_indirect_edges_after_inlining (st
> ? ? ? if (new_direct_edge)
> ? ? ? ?{
> ? ? ? ? ?new_direct_edge->indirect_inlining_edge = 1;
> - ? ? ? ? if (new_edges)
> + ? ? ? ? if (new_edges && new_direct_edge->callee != ignore)
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?VEC_safe_push (cgraph_edge_p, heap, *new_edges,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? new_direct_edge);
> @@ -1612,22 +1613,24 @@ update_indirect_edges_after_inlining (st
> ? ?update_indirect_edges_after_inlining on all nodes and
> ? ?update_jump_functions_after_inlining on all non-inlined edges that lead out
> ? ?of this subtree. ?Newly discovered indirect edges will be added to
> - ? *NEW_EDGES, unless NEW_EDGES is NULL. ?Return true iff a new edge(s) were
> - ? created. ?*/
> + ? *NEW_EDGES except when NEW_EDGES is NULL or the edge leads to the node
> + ? IGNORE. ?Return true iff a new edge(s) were added to the vector. ?*/
>
> ?static bool
> ?propagate_info_to_inlined_callees (struct cgraph_edge *cs,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct cgraph_node *node,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct cgraph_node *ignore,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VEC (cgraph_edge_p, heap) **new_edges)
> ?{
> ? struct cgraph_edge *e;
> ? bool res;
>
> - ?res = update_indirect_edges_after_inlining (cs, node, new_edges);
> + ?res = update_indirect_edges_after_inlining (cs, node, ignore, new_edges);
>
> ? for (e = node->callees; e; e = e->next_callee)
> ? ? if (!e->inline_failed)
> - ? ? ?res |= propagate_info_to_inlined_callees (cs, e->callee, new_edges);
> + ? ? ?res |= propagate_info_to_inlined_callees (cs, e->callee, ignore,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? new_edges);
> ? ? else
> ? ? ? update_jump_functions_after_inlining (cs, e);
>
> @@ -1637,13 +1640,16 @@ propagate_info_to_inlined_callees (struc
> ?/* Update jump functions and call note functions on inlining the call site CS.
> ? ?CS is expected to lead to a node already cloned by
> ? ?cgraph_clone_inline_nodes. ?Newly discovered indirect edges will be added to
> - ? *NEW_EDGES, unless NEW_EDGES is NULL. ?Return true iff a new edge(s) were +
> - ? created. ?*/
> + ? *NEW_EDGES except when NEW_EDGES is NULL or the edge leads to the node
> + ? IGNORE. ?Return true iff any new edges were added to the vector. ?*/
>
> ?bool
> ?ipa_propagate_indirect_call_infos (struct cgraph_edge *cs,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?bool new_recursive,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VEC (cgraph_edge_p, heap) **new_edges)
> ?{
> + ?struct cgraph_node *ignore;
> +
> ? /* FIXME lto: We do not stream out indirect call information. ?*/
> ? if (flag_wpa)
> ? ? return false;
> @@ -1654,7 +1660,16 @@ ipa_propagate_indirect_call_infos (struc
> ? ? return false;
> ? gcc_assert (ipa_edge_args_vector);
>
> - ?return propagate_info_to_inlined_callees (cs, cs->callee, new_edges);
> + ?if (new_recursive)
> + ? ?ignore = NULL;
> + ?else
> + ? ?{
> + ? ? ?ignore = cs->caller;
> + ? ? ?if (ignore->global.inlined_to)
> + ? ? ? ignore = ignore->global.inlined_to;
> + ? ?}
> +
> + ?return propagate_info_to_inlined_callees (cs, cs->callee, ignore, new_edges);
> ?}
>
> ?/* Frees all dynamically allocated structures that the argument info points
> Index: icln/gcc/ipa-prop.h
> ===================================================================
> --- icln.orig/gcc/ipa-prop.h
> +++ icln/gcc/ipa-prop.h
> @@ -427,6 +427,7 @@ void ipa_analyze_node (struct cgraph_nod
> ?/* Function formal parameters related computations. ?*/
> ?void ipa_initialize_node_params (struct cgraph_node *node);
> ?bool ipa_propagate_indirect_call_infos (struct cgraph_edge *cs,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool new_recursive,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?VEC (cgraph_edge_p, heap) **new_edges);
>
> ?/* Indirect edge and binfo processing. ?*/
> Index: icln/gcc/testsuite/g++.dg/torture/pr45572-1.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/torture/pr45572-1.C
> @@ -0,0 +1,63 @@
> +// { dg-do compile }
> +
> +extern "C" {
> +typedef long unsigned int size_t;
> +typedef long int __ssize_t;
> +typedef struct _IO_FILE FILE;
> +typedef struct
> +{
> +} __mbstate_t;
> +extern __inline __attribute__ ((__gnu_inline__)) int
> +fgetc_unlocked (FILE *__fp)
> +{
> +}
> +extern __inline __attribute__ ((__gnu_inline__)) int
> +putc_unlocked (int __c, FILE *__stream)
> +{
> +}
> +extern __inline __attribute__ ((__gnu_inline__)) __ssize_t
> +getline (char **__lineptr, size_t *__n, FILE *__stream)
> +{
> +}
> +extern __inline __attribute__ ((__gnu_inline__)) int
> +ferror_unlocked (FILE *__stream) throw ()
> +{
> +}
> +}
> +typedef struct
> +{} __mpf_struct;
> +typedef __mpf_struct mpf_t[1];
> +typedef const __mpf_struct *mpf_srcptr;
> +typedef __mpf_struct *mpf_ptr;
> +extern "C" {
> + void __gmpf_add (mpf_ptr, mpf_srcptr, mpf_srcptr);
> +}
> +class _knumber
> +{
> + public:
> + ?enum NumType {SpecialType, IntegerType, FractionType, FloatType};
> + ?virtual NumType type(void) const = 0;
> + ?virtual _knumber * add(_knumber const & arg2) const = 0;
> + ?virtual operator long int(void) const = 0;
> +};
> +class _knumfloat : public _knumber
> +{
> + ?_knumfloat(double num = 1.0)
> + ?;
> + ?virtual NumType type(void) const ;
> + ?virtual _knumber * add(_knumber const & arg2) const;
> + ?virtual operator long int (void) const;
> + ? ?mpf_t _mpf;
> +};
> +_knumber *_knumfloat::add(_knumber const & arg2) const
> +{
> + ?if (arg2.type() == SpecialType)
> + ? ?return arg2.add(*this);
> +{
> + ? ?_knumfloat tmp_num(arg2);
> + ? ?return tmp_num.add(*this);
> + ?}
> + ?_knumfloat * tmp_num = new _knumfloat();
> + ?__gmpf_add(tmp_num->_mpf, _mpf,
> + ? dynamic_cast<_knumfloat const &>(arg2)._mpf);
> +}
> Index: icln/gcc/testsuite/g++.dg/torture/pr45572-2.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/torture/pr45572-2.C
> @@ -0,0 +1,39 @@
> +// { dg-do compile }
> +
> +typedef struct
> +{} __mpf_struct;
> +typedef __mpf_struct mpf_t[1];
> +typedef const __mpf_struct *mpf_srcptr;
> +typedef __mpf_struct *mpf_ptr;
> +extern "C" {
> + void __gmpf_add (mpf_ptr, mpf_srcptr, mpf_srcptr);
> +}
> +class _knumber
> +{
> + public:
> + ?enum NumType {SpecialType, IntegerType, FractionType, FloatType};
> + ?virtual NumType type(void) const = 0;
> + ?virtual _knumber * add(_knumber const & arg2) const = 0;
> + ?virtual operator long int(void) const = 0;
> +};
> +class _knumfloat : public _knumber
> +{
> + ?_knumfloat(double num = 1.0)
> + ?;
> + ?virtual NumType type(void) const ;
> + ?virtual _knumber * add(_knumber const & arg2) const;
> + ?virtual operator long int (void) const;
> + ? ?mpf_t _mpf;
> +};
> +_knumber *_knumfloat::add(_knumber const & arg2) const
> +{
> + ?if (arg2.type() == SpecialType)
> + ? ?return arg2.add(*this);
> +{
> + ? ?_knumfloat tmp_num(arg2);
> + ? ?return tmp_num.add(*this);
> + ?}
> + ?_knumfloat * tmp_num = new _knumfloat();
> + ?__gmpf_add(tmp_num->_mpf, _mpf,
> + ? dynamic_cast<_knumfloat const &>(arg2)._mpf);
> +}
>
>