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

Martin Jambor mjambor@suse.cz
Wed Sep 22 23:37:00 GMT 2010 

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

More information about the Gcc-patches mailing list