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

[Martin Jambor <mjambor at suse dot cz>]

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)
>      {
>