tidy some gimplifcation unsharing

tidy some gimplifcation unsharing

[Richard Henderson]

The few places that I "avoid" copying decls, types, etc, doesn't reduce
copies but merely stops sooner than copy_node before not copying them.

The VA_ARG_EXPR thing is handled during gimplification elsewhere.

The BIND_EXPR thing avoids unnecessary copying of DECL_INITIALs, as
described in the block comment.  This is necessary for some patches
I'm working on, in which those DECL_INITIALs might not be copyable.

Doing that exposed a latent problem with sharing of TYPE_SIZE_UNIT
when it was used.

Tested on i686-linux.


r~


        * gimplify.c (mostly_copy_tree_r): Don't attempt to copy decls.
        (copy_if_shared_r): Don't copy decls, types, constants, BINDs.
        Don't mark VA_ARG_EXPRs volatile here.
        (gimplify_modify_expr): Unshare TYPE_SIZE_UNIT.

Index: gimplify.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimplify.c,v
retrieving revision 2.4
diff -u -p -r2.4 gimplify.c
--- gimplify.c	20 May 2004 17:37:02 -0000	2.4
+++ gimplify.c	21 May 2004 20:17:26 -0000
@@ -588,8 +588,9 @@ static tree
 mostly_copy_tree_r (tree *tp, int *walk_subtrees, void *data)
 {
   enum tree_code code = TREE_CODE (*tp);
-  /* Don't unshare types, constants and SAVE_EXPR nodes.  */
+  /* Don't unshare types, decls, constants and SAVE_EXPR nodes.  */
   if (TREE_CODE_CLASS (code) == 't'
+      || TREE_CODE_CLASS (code) == 'd'
       || TREE_CODE_CLASS (code) == 'c'
       || code == SAVE_EXPR || code == TARGET_EXPR
       /* We can't do anything sensible with a BLOCK used as an expression,
@@ -632,26 +633,40 @@ static tree
 copy_if_shared_r (tree *tp, int *walk_subtrees ATTRIBUTE_UNUSED,
 		  void *data ATTRIBUTE_UNUSED)
 {
+  tree t = *tp;
+  enum tree_code code = TREE_CODE (t);
+
+  /* Skip types, decls, and constants.  */
+  if (TREE_CODE_CLASS (code) == 't'
+      || TREE_CODE_CLASS (code) == 'd'
+      || TREE_CODE_CLASS (code) == 'c')
+    *walk_subtrees = 0;
+
+  /* Special-case BIND_EXPR.  We should never be copying these, therefore
+     we can omit examining BIND_EXPR_VARS.  Which also avoids problems with
+     double processing of the DECL_INITIAL, which could be seen via both
+     the BIND_EXPR_VARS and a DECL_STMT.  */
+  else if (code == BIND_EXPR)
+    {
+      if (TREE_VISITED (t))
+	abort ();
+      TREE_VISITED (t) = 1;
+      *walk_subtrees = 0;
+      walk_tree (&BIND_EXPR_BODY (t), copy_if_shared_r, NULL, NULL);
+    }
+
   /* If this node has been visited already, unshare it and don't look
      any deeper.  */
-  if (TREE_VISITED (*tp))
+  else if (TREE_VISITED (t))
     {
       walk_tree (tp, mostly_copy_tree_r, NULL, NULL);
       *walk_subtrees = 0;
     }
+
+  /* Otherwise, mark the tree as visited and keep looking.  */
   else
-    {
-    /* Otherwise, mark the tree as visited and keep looking.  */
-    TREE_VISITED (*tp) = 1;
-      if (TREE_CODE (*tp) == VA_ARG_EXPR)
-	{
-	  /* Mark any _DECL inside the operand as volatile to avoid the
-	     optimizers messing around with it. FIXME: Remove this once
-	     VA_ARG_EXPRs are properly lowered.  */
-	  walk_tree (&TREE_OPERAND (*tp, 0), mark_decls_volatile_r,
-		     NULL, NULL);
-	}
-    }
+    TREE_VISITED (t) = 1;
+
   return NULL_TREE;
 }
 
@@ -2472,6 +2487,7 @@ gimplify_modify_expr (tree *expr_p, tree
 	  tree args, t, dest;
 
 	  t = TYPE_SIZE_UNIT (TREE_TYPE (*to_p));
+	  t = unshare_expr (t);
 	  args = tree_cons (NULL, t, NULL);
 	  t = build_addr_expr (*from_p);
 	  args = tree_cons (NULL, t, args);

Re: tidy some gimplifcation unsharing

[Alexandre Oliva]

On May 21, 2004, Richard Henderson <rth@redhat.com> wrote:

> The VA_ARG_EXPR thing is handled during gimplification elsewhere.

But that's too late.  We may have already expanded statements assuming
variables referenced within VA_ARG_EXPRs are not volatile, in which
case the stmts will fail gimple verification later.  This broke the
build of uClibc for frv-uclinux.

>         Don't mark VA_ARG_EXPRs volatile here.

Ok to revert this bit?

Actually...  There should be a comment explaining why it's there, to
avoid having someone else make the same mistake.  Ok to install this,
if it bootstraps?  I'll post a proper patch, with a ChangeLog entry,
when I'm done testing.

What I'm not sure is whether it would be reasonable to remove the
equivalent call from gimplify_expr().  What do you think?

While at that...  Is it really the case that VA_ARG_EXPR can take
arbitrarily complex lvalue expressions?  Consider, for example,
foo->bar[baz]->saved_va_list.  Shouldn't we even attempt to gimplify the
expression as an lvalue, and, while at that, do so before marking the
decls found in the expression?  I don't think we should be changing
global decls just because they happen to be mentioned in the argument
to va_arg.  Am I missing anything?


--- gimplify.c.~2.6.~	2004-05-28 01:36:45.000000000 -0300
+++ gimplify.c	2004-05-28 06:21:16.000000000 -0300
@@ -665,7 +665,19 @@ copy_if_shared_r (tree *tp, int *walk_su
 
   /* Otherwise, mark the tree as visited and keep looking.  */
   else
-    TREE_VISITED (t) = 1;
+    {
+      TREE_VISITED (t) = 1;
+      if (TREE_CODE (*tp) == VA_ARG_EXPR)
+	{
+	  /* Mark any _DECL inside the operand as volatile to avoid
+	     the optimizers messing around with it. We have to do this
+	     early, otherwise we might mark a variable as volatile
+	     after we gimplify other statements that use the variable
+	     assuming it's not volatile.  */
+	  walk_tree (&TREE_OPERAND (*tp, 0), mark_decls_volatile_r,
+		     NULL, NULL);
+	}
+    }
 
   return NULL_TREE;
 }


-- 
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: tidy some gimplifcation unsharing

[Alexandre Oliva]

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

On May 28, 2004, Alexandre Oliva <aoliva@redhat.com> wrote:

>> Don't mark VA_ARG_EXPRs volatile here.

> Ok to revert this bit?

Here's a testcase that regressed:


[-- Attachment #2: va-list-bug.c --]
[-- Type: application/octet-stream, Size: 148 bytes --]

#include <stdarg.h>

struct argp {
  va_list arg;
};

char *f(struct argp *p)
{
  char *q = (char *)p;

  q += va_arg (p->arg, int);

  return q;
}

[-- Attachment #3: 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: tidy some gimplifcation unsharing

[Diego Novillo]

On Fri, 2004-05-21 at 18:05, Richard Henderson wrote:

> The VA_ARG_EXPR thing is handled during gimplification elsewhere.
> 
Not always.  Remember PR14498.  Dale had to add this back in March,
because the gimplifier was seeing the VA_ARG_EXPR after the variable had
already been gimplified as a register.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14498 has a test case.  We
really need to lower VA_ARG_EXPR.  This is just too ugly.


Diego.

Re: tidy some gimplifcation unsharing

[Diego Novillo]

On Fri, 2004-05-28 at 09:58, Diego Novillo wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14498 has a test case.  We
> really need to lower VA_ARG_EXPR.  This is just too ugly.
> 
Which reminds me.  Dale, did you ever add this to the testsuite?  If
not, could you add it now?


Thanks.  Diego.

Re: tidy some gimplifcation unsharing

[Dale Johannesen]

On May 28, 2004, at 7:01 AM, Diego Novillo wrote:
> On Fri, 2004-05-28 at 09:58, Diego Novillo wrote:
>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14498 has a test case.  We
>> really need to lower VA_ARG_EXPR.  This is just too ugly.
>>
> Which reminds me.  Dale, did you ever add this to the testsuite?  If
> not, could you add it now?

Apparently not.  OK, it's in (mainline).

Re: tidy some gimplifcation unsharing

[Alexandre Oliva]

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

On May 28, 2004, Alexandre Oliva <aoliva@redhat.com> wrote:

> On May 21, 2004, Richard Henderson <rth@redhat.com> wrote:
>> The VA_ARG_EXPR thing is handled during gimplification elsewhere.

> But that's too late.  We may have already expanded statements assuming
> variables referenced within VA_ARG_EXPRs are not volatile, in which
> case the stmts will fail gimple verification later.  This broke the
> build of uClibc for frv-uclinux.

>> Don't mark VA_ARG_EXPRs volatile here.

> Ok to revert this bit?

Ping?

> Actually...  There should be a comment explaining why it's there, to
> avoid having someone else make the same mistake.  Ok to install this,
> if it bootstraps?

It does.

> I'll post a proper patch, with a ChangeLog entry, when I'm done
> testing.

Oops, I didn't post it.  Here it is.  Ok to install?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gimplify-mark-va-arg-expr-decls-early-as-volatile.patch --]
[-- Type: text/x-patch, Size: 1153 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* gimplify.c (copy_if_shared_r): Revert:
	2004-05-21  Richard Henderson  <rth@redhat.com>
	* gimplify.c [...] Don't mark VA_ARG_EXPRs volatile here.

Index: gcc/gimplify.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimplify.c,v
retrieving revision 2.10
diff -u -p -r2.10 gimplify.c
--- gcc/gimplify.c 30 May 2004 18:32:26 -0000 2.10
+++ gcc/gimplify.c 7 Jun 2004 19:10:56 -0000
@@ -665,7 +665,19 @@ copy_if_shared_r (tree *tp, int *walk_su
 
   /* Otherwise, mark the tree as visited and keep looking.  */
   else
-    TREE_VISITED (t) = 1;
+    {
+      TREE_VISITED (t) = 1;
+      if (TREE_CODE (*tp) == VA_ARG_EXPR)
+	{
+	  /* Mark any _DECL inside the operand as volatile to avoid
+	     the optimizers messing around with it. We have to do this
+	     early, otherwise we might mark a variable as volatile
+	     after we gimplify other statements that use the variable
+	     assuming it's not volatile.  */
+	  walk_tree (&TREE_OPERAND (*tp, 0), mark_decls_volatile_r,
+		     NULL, NULL);
+	}
+    }
 
   return NULL_TREE;
 }

[-- Attachment #3: 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: tidy some gimplifcation unsharing

[Richard Henderson]

On Mon, Jun 07, 2004 at 04:12:39PM -0300, Alexandre Oliva wrote:
> 	* gimplify.c (copy_if_shared_r): Revert:
> 	2004-05-21  Richard Henderson  <rth@redhat.com>
> 	* gimplify.c [...] Don't mark VA_ARG_EXPRs volatile here.

I guess.  Better perhaps would be to do this in the C/C++ front
ends here the VA_ARG_EXPRs are created in the first place.


r~