-funit-at-a-time x nested fns x inlining of enclosing fn

-funit-at-a-time x nested fns x inlining of enclosing fn

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

The following minimal testcase:

static inline int g(void) {
  auto int h(void) __attribute__((noinline));
  int h(void) { return 1; }
  return h();
}
int f(void) { return g(); }

fails to compile with current mainline.  The problem is that when we
remove the node for function g, we take the node for function g::h
with it, and then we lose track of h's (not) inlininig info.

Here's a testcase that's a bit more interesting, in that it shows that
some trivial fixes for the problem won't work:

static inline int g(int i) {
  auto inline int h(void);
  auto int h2(void) __attribute__((noinline));
  int h(void) { return i+1; }
  int h2(void) { return h(); }
  return h2();
}
int f(void) { return g(1); }
int f2(void) { return g(1); }


Here are two candidate patches that remove the failure.  The first one
arranges for us to always clone functions we're going to inline, if
they have nested functions.  It must adjust other sanity checks that
would report the unused originally-cloned function as something that
should have been removed.  The tests are probably too lax, and might
let through actual error conditions.

So I wrote the second patch.  It removes the node for the enclosing
function but it keeps nodes for nested functions if they have callers
left after the edge from the parent is removed.

I suppose this might cause nodes to be left in if nested functions
call each other, or are just processed in the wrong order, or have
nested functions themselves.  Also, the nested functions would remain
pointing to the removed nodes corresponding to their enclosing
functions.  This doesn't seem to be a problem, but the other issues
might be, so this is not a complete solution either.

I'm not entirely happy with any of them, but I'd appreciate thoughts
on how to best deal with this problem.

In case you're wondering, the original testcase was
glibc/locale/programs/ld-ctype.c.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cgraph-nested.patch --]
[-- Type: text/x-patch, Size: 2068 bytes --]

Index: cgraphunit.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cgraphunit.c,v
retrieving revision 1.68
diff -u -p -r1.68 cgraphunit.c
--- cgraphunit.c 1 Jul 2004 07:51:11 -0000 1.68
+++ cgraphunit.c 9 Jul 2004 11:32:00 -0000
@@ -770,7 +770,7 @@ cgraph_mark_functions_to_output (void)
 	node->output = 1;
       /* We should've reclaimed all functions that are not needed.  */
       else if (!node->global.inlined_to && DECL_SAVED_TREE (decl)
-	       && !DECL_EXTERNAL (decl))
+	       && !DECL_EXTERNAL (decl) && !node->nested)
 	{
 	  dump_cgraph_node (stderr, node);
 	  abort ();
@@ -947,7 +947,10 @@ cgraph_remove_unreachable_nodes (void)
 	  if (cgraph_dump_file)
 	    fprintf (cgraph_dump_file, " %s", cgraph_node_name (node));
 	  if (!node->analyzed || !DECL_EXTERNAL (node->decl))
-	    cgraph_remove_node (node);
+	    {
+	      if (!node->nested)
+		cgraph_remove_node (node);
+	    }
 	  else
 	    {
 	      struct cgraph_edge *e;
@@ -1029,10 +1032,14 @@ cgraph_clone_inlined_nodes (struct cgrap
 {
   struct cgraph_node *n;
 
-  /* We may eliminate the need for out-of-line copy to be output.  In that
-     case just go ahead and re-use it.  */
+  /* We may eliminate the need for out-of-line copy to be output.  In
+     that case just go ahead and re-use it.  Make sure we don't re-use
+     it if it contains nested functions, otherwise when we remove the
+     node after inlining we'll remove the nested nodes as well, and
+     they might have to remain.  */
   if (!e->callee->callers->next_caller
       && (!e->callee->needed || DECL_EXTERNAL (e->callee->decl))
+      && !e->callee->nested
       && duplicate
       && flag_unit_at_a_time)
     {
@@ -1776,7 +1783,8 @@ cgraph_optimize (void)
       for (node = cgraph_nodes; node; node = node->next)
 	if (node->analyzed
 	    && (node->global.inlined_to
-	        || DECL_SAVED_TREE (node->decl)))
+	        || DECL_SAVED_TREE (node->decl))
+	    && !node->nested)
 	  {
 	    error_found = true;
 	    dump_cgraph_node (stderr, node);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: cgraph-nested-take2.patch --]
[-- Type: text/x-patch, Size: 803 bytes --]

Index: cgraph.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cgraph.c,v
retrieving revision 1.53
diff -u -p -r1.53 cgraph.c
--- cgraph.c 30 May 2004 07:12:48 -0000 1.53
+++ cgraph.c 9 Jul 2004 11:46:56 -0000
@@ -304,8 +304,20 @@ cgraph_remove_node (struct cgraph_node *
     cgraph_remove_edge (node->callers);
   while (node->callees)
     cgraph_remove_edge (node->callees);
-  while (node->nested)
-    cgraph_remove_node (node->nested);
+
+  {
+    struct cgraph_node *next = node->nested;
+
+    while (next)
+      {
+	struct cgraph_node *nested = next;
+
+	next = nested->next_nested;
+	if (!nested->callers)
+	  cgraph_remove_node (nested);
+      }
+  }
+	
   if (node->origin)
     {
       struct cgraph_node **node2 = &node->origin->nested;

[-- Attachment #4: Type: text/plain, Size: 188 bytes --]


-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: -funit-at-a-time x nested fns x inlining of enclosing fn

[Alexandre Oliva]

On Jul  9, 2004, Alexandre Oliva <aoliva@redhat.com> wrote:

> I'm not entirely happy with any of them

Especially because both fail with -O2 -g :-/

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: -funit-at-a-time x nested fns x inlining of enclosing fn

[Richard Henderson]

On Fri, Jul 09, 2004 at 09:08:17AM -0300, Alexandre Oliva wrote:
> I'm not entirely happy with any of them, but I'd appreciate thoughts
> on how to best deal with this problem.

After function un-nesting, I see no reason to keep the parent/child
relationship in cgraph.  No idea what happens with dwarf2out though...


r~

Re: -funit-at-a-time x nested fns x inlining of enclosing fn

[Andrew Pinski]

> 
> --=-=-=
> 
> The following minimal testcase:
> 
> static inline int g(void) {
>   auto int h(void) __attribute__((noinline));
>   int h(void) { return 1; }
>   return h();
> }
> int f(void) { return g(); }
> 
> fails to compile with current mainline.  The problem is that when we
> remove the node for function g, we take the node for function g::h
> with it, and then we lose track of h's (not) inlininig info.

I should note this is PR 16460.

Thanks,
Andrew Pinski

Re: -funit-at-a-time x nested fns x inlining of enclosing fn

[Jan Hubicka]

> On Fri, Jul 09, 2004 at 09:08:17AM -0300, Alexandre Oliva wrote:
> > I'm not entirely happy with any of them, but I'd appreciate thoughts
> > on how to best deal with this problem.
> 
> After function un-nesting, I see no reason to keep the parent/child
> relationship in cgraph.  No idea what happens with dwarf2out though...

I have to take a look into it.
One place where parent-child relationship is needed are the nested
functions of extern inline functions.
Tought these are static we must not emit these unless we inlined the
origin unlike normal static functions in non-unit-at-a-time mode.
This can be definitly cared by flag instead of preserving the whole
relationship chain.

Honza
> 
> 
> r~

Re: -funit-at-a-time x nested fns x inlining of enclosing fn

[Jason Merrill]

On Sat, 10 Jul 2004 12:23:33 +0200, Jan Hubicka <jh@suse.cz> wrote:

> One place where parent-child relationship is needed are the nested
> functions of extern inline functions.
> Tought these are static we must not emit these unless we inlined the
> origin unlike normal static functions in non-unit-at-a-time mode.

Hmm?  It seems to me that we need a copy iff we emit a call to the
function.  How is this different from other statics?

Jason

Re: -funit-at-a-time x nested fns x inlining of enclosing fn

[Jan Hubicka]

> On Sat, 10 Jul 2004 12:23:33 +0200, Jan Hubicka <jh@suse.cz> wrote:
> 
> > One place where parent-child relationship is needed are the nested
> > functions of extern inline functions.
> > Tought these are static we must not emit these unless we inlined the
> > origin unlike normal static functions in non-unit-at-a-time mode.
> 
> Hmm?  It seems to me that we need a copy iff we emit a call to the
> function.  How is this different from other statics?

In non-unit-at-a-time we emit offline copy of all non-inline static
functions, while emmiting of nested inline functions has been controled
by inlineness of the outermost origin instead of function itself.  There
are tests in the testsuite for this behaviour and definitly in the
context of extern inline it is only sane implementation of this crazy
combinations of extensions.

What about simply deferring each inlined function?  I am traveling right
now, but I will make patch for this in two days or so.

Honza
> 
> Jason