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

Hi,

On Wed, Sep 22, 2010 at 12:20:18PM +0200, Jan Hubicka wrote:
> > 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.
> 
> This is the case where we have previously virtual call but
> devirtualization finds call using object with external vtable (so
> function never seen previously becomes reachable).
> 
> I don't like the approach of keeping calls indirect when they are
> really known to be direct.  Even when it won't degrade inlining
> (since the function was unseen previously and thus has no body),
> still how much pain would be to update tables instead?

It is as easy as calling ipa_check_create_node_params after
cgraph_node().  I just did not want to reallocate the vectors
needlessly.  The new patch below does that.

> 
> > 
> > > 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?).
> 
> I saw here a testcase where virtual method was self recursive. Only after
> inlining the first call the self recursiveness become obvious, so I guess
> we should handle this.
> 
> I looked into the PR briefly before and noticed it has an weird consequences,
> but didn't quite understood why?
> 

Yesterday I had a theory what was going on but I was wrong.  The
problem was simply that in add_new_edges_to_heap we did:

      if (edge->callee->local.inlinable
	  && cgraph_default_inline_p (edge->callee, &edge->inline_failed))

which overwrote edge->inline_failed with a non-zero, making an already
inlined edge an un-inlined one.  I think this was then usually
directly caught by the verifier but potentially other mischief could
happen, like redirecting such edges to master_clones, resulting in
dangling inlined_to nodes and so on and so forth. 

It took me quite a while to realize what was going on, the line looked
so innocent.

Anyway, here is a much simpler patch which also works.  Bootstrapped
and tested on x86_64-linux.  OK for trunk?



2010-09-22  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/45572
	* ipa-prop.c (ipa_make_edge_direct_to_target): Call
	ipa_check_create_node_params.
	* ipa-inline.c (add_new_edges_to_heap): Do not insert inlined edges.

	* testsuite/g++.dg/ipa/pr45572-1.C: New test.
	* testsuite/g++.dg/ipa/pr45572-2.C: Likewise.

Ping.

Thanks,

Martin

On Wed, Sep 22, 2010 at 08:30:57PM +0200, Martin Jambor wrote:
> Hi,
> 
> On Wed, Sep 22, 2010 at 12:20:18PM +0200, Jan Hubicka wrote:
> > > 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.
> > 
> > This is the case where we have previously virtual call but
> > devirtualization finds call using object with external vtable (so
> > function never seen previously becomes reachable).
> > 
> > I don't like the approach of keeping calls indirect when they are
> > really known to be direct.  Even when it won't degrade inlining
> > (since the function was unseen previously and thus has no body),
> > still how much pain would be to update tables instead?
> 
> It is as easy as calling ipa_check_create_node_params after
> cgraph_node().  I just did not want to reallocate the vectors
> needlessly.  The new patch below does that.
> 
> > 
> > > 
> > > > 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?).
> > 
> > I saw here a testcase where virtual method was self recursive. Only after
> > inlining the first call the self recursiveness become obvious, so I guess
> > we should handle this.
> > 
> > I looked into the PR briefly before and noticed it has an weird consequences,
> > but didn't quite understood why?
> > 
> 
> Yesterday I had a theory what was going on but I was wrong.  The
> problem was simply that in add_new_edges_to_heap we did:
> 
>       if (edge->callee->local.inlinable
> 	  && cgraph_default_inline_p (edge->callee, &edge->inline_failed))
> 
> which overwrote edge->inline_failed with a non-zero, making an already
> inlined edge an un-inlined one.  I think this was then usually
> directly caught by the verifier but potentially other mischief could
> happen, like redirecting such edges to master_clones, resulting in
> dangling inlined_to nodes and so on and so forth. 
> 
> It took me quite a while to realize what was going on, the line looked
> so innocent.
> 
> Anyway, here is a much simpler patch which also works.  Bootstrapped
> and tested on x86_64-linux.  OK for trunk?
> 
> 
> 
> 2010-09-22  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/45572
> 	* ipa-prop.c (ipa_make_edge_direct_to_target): Call
> 	ipa_check_create_node_params.
> 	* ipa-inline.c (add_new_edges_to_heap): Do not insert inlined edges.
> 
> 	* testsuite/g++.dg/ipa/pr45572-1.C: New test.
> 	* testsuite/g++.dg/ipa/pr45572-2.C: Likewise.
> 
> Index: icln/gcc/testsuite/g++.dg/ipa/pr45572-1.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/ipa/pr45572-1.C
> @@ -0,0 +1,64 @@
> +// { dg-do compile }
> +// { dg-options "-finline-small-functions -findirect-inlining -finline-functions -O" }
> +
> +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/ipa/pr45572-2.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/ipa/pr45572-2.C
> @@ -0,0 +1,39 @@
> +// { dg-do compile }
> +// { dg-options "-finline-small-functions -findirect-inlining -finline-function+
> +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/ipa-inline.c
> ===================================================================
> --- icln.orig/gcc/ipa-inline.c
> +++ icln/gcc/ipa-inline.c
> @@ -1031,6 +1031,7 @@ add_new_edges_to_heap (fibheap_t heap, V
>  
>        gcc_assert (!edge->aux);
>        if (edge->callee->local.inlinable
> +	  && edge->inline_failed
>  	  && cgraph_default_inline_p (edge->callee, &edge->inline_failed))
>          edge->aux = fibheap_insert (heap, cgraph_edge_badness (edge, false), edge);
>      }
> Index: icln/gcc/ipa-prop.c
> ===================================================================
> --- icln.orig/gcc/ipa-prop.c
> +++ icln/gcc/ipa-prop.c
> @@ -1447,7 +1447,7 @@ ipa_make_edge_direct_to_target (struct c
>    callee = cgraph_node (target);
>    if (!callee)
>      return NULL;
> -
> +  ipa_check_create_node_params ();
>    cgraph_make_edge_direct (ie, callee);
>    if (dump_file)
>      {
>

On Fri, Oct 1, 2010 at 10:26 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Ping.

Ok.

Thanks,
Richard.

> Thanks,
>
> Martin
>
> On Wed, Sep 22, 2010 at 08:30:57PM +0200, Martin Jambor wrote:
>> Hi,
>>
>> On Wed, Sep 22, 2010 at 12:20:18PM +0200, Jan Hubicka wrote:
>> > > 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.
>> >
>> > This is the case where we have previously virtual call but
>> > devirtualization finds call using object with external vtable (so
>> > function never seen previously becomes reachable).
>> >
>> > I don't like the approach of keeping calls indirect when they are
>> > really known to be direct.  Even when it won't degrade inlining
>> > (since the function was unseen previously and thus has no body),
>> > still how much pain would be to update tables instead?
>>
>> It is as easy as calling ipa_check_create_node_params after
>> cgraph_node().  I just did not want to reallocate the vectors
>> needlessly.  The new patch below does that.
>>
>> >
>> > >
>> > > > 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?).
>> >
>> > I saw here a testcase where virtual method was self recursive. Only after
>> > inlining the first call the self recursiveness become obvious, so I guess
>> > we should handle this.
>> >
>> > I looked into the PR briefly before and noticed it has an weird consequences,
>> > but didn't quite understood why?
>> >
>>
>> Yesterday I had a theory what was going on but I was wrong.  The
>> problem was simply that in add_new_edges_to_heap we did:
>>
>>       if (edge->callee->local.inlinable
>>         && cgraph_default_inline_p (edge->callee, &edge->inline_failed))
>>
>> which overwrote edge->inline_failed with a non-zero, making an already
>> inlined edge an un-inlined one.  I think this was then usually
>> directly caught by the verifier but potentially other mischief could
>> happen, like redirecting such edges to master_clones, resulting in
>> dangling inlined_to nodes and so on and so forth.
>>
>> It took me quite a while to realize what was going on, the line looked
>> so innocent.
>>
>> Anyway, here is a much simpler patch which also works.  Bootstrapped
>> and tested on x86_64-linux.  OK for trunk?
>>
>>
>>
>> 2010-09-22  Martin Jambor  <mjambor@suse.cz>
>>
>>       PR tree-optimization/45572
>>       * ipa-prop.c (ipa_make_edge_direct_to_target): Call
>>       ipa_check_create_node_params.
>>       * ipa-inline.c (add_new_edges_to_heap): Do not insert inlined edges.
>>
>>       * testsuite/g++.dg/ipa/pr45572-1.C: New test.
>>       * testsuite/g++.dg/ipa/pr45572-2.C: Likewise.
>>
>> Index: icln/gcc/testsuite/g++.dg/ipa/pr45572-1.C
>> ===================================================================
>> --- /dev/null
>> +++ icln/gcc/testsuite/g++.dg/ipa/pr45572-1.C
>> @@ -0,0 +1,64 @@
>> +// { dg-do compile }
>> +// { dg-options "-finline-small-functions -findirect-inlining -finline-functions -O" }
>> +
>> +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/ipa/pr45572-2.C
>> ===================================================================
>> --- /dev/null
>> +++ icln/gcc/testsuite/g++.dg/ipa/pr45572-2.C
>> @@ -0,0 +1,39 @@
>> +// { dg-do compile }
>> +// { dg-options "-finline-small-functions -findirect-inlining -finline-function+
>> +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/ipa-inline.c
>> ===================================================================
>> --- icln.orig/gcc/ipa-inline.c
>> +++ icln/gcc/ipa-inline.c
>> @@ -1031,6 +1031,7 @@ add_new_edges_to_heap (fibheap_t heap, V
>>
>>        gcc_assert (!edge->aux);
>>        if (edge->callee->local.inlinable
>> +       && edge->inline_failed
>>         && cgraph_default_inline_p (edge->callee, &edge->inline_failed))
>>          edge->aux = fibheap_insert (heap, cgraph_edge_badness (edge, false), edge);
>>      }
>> Index: icln/gcc/ipa-prop.c
>> ===================================================================
>> --- icln.orig/gcc/ipa-prop.c
>> +++ icln/gcc/ipa-prop.c
>> @@ -1447,7 +1447,7 @@ ipa_make_edge_direct_to_target (struct c
>>    callee = cgraph_node (target);
>>    if (!callee)
>>      return NULL;
>> -
>> +  ipa_check_create_node_params ();
>>    cgraph_make_edge_direct (ie, callee);
>>    if (dump_file)
>>      {
>>
>