This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: GCC Status Report (2004-03-09)


> >Do you want me to revert that patch?
>
> Yes -- if your tests confirm that it is safe to do so.

The attached patch successfully completed a testing cycle for the 3.4 branch 
on x86 (including the testcase for PR opt/8634) but I'm not convinced it is 
safe because, even if the comment in maybe_set_unchanging seems to imply 
that the problem for PR opt/8634 was caused by the double store in 
store_constructor, this is not the case: the original problem came from the 
mere existence of /u on memory writes for non-static initializers.  See
http://gcc.gnu.org/ml/gcc-patches/2002-12/msg00533.html
http://gcc.gnu.org/ml/gcc-patches/2003-04/msg00482.html
http://gcc.gnu.org/ml/gcc-patches/2003-04/msg00489.html

And I'm not sure whether rth's patch catches all the cases.

> It's supposed to be the RTL equivalent of "const".  In other words, once
> initialized, an RTX_UNCHANGING_P thing is immutable, and therefore no
> writes can alias it.  If you're after the one write to the
> RTX_UNCHANGING_P thing, then it's value is always valid.  That's true
> even if someone has its address; they cannot write through the pointer.

Yes, I understand the definition.  The problem for me is really the purpose.

> This is clearly a valuable optimization aid.

Optimization or correctness?  We had several bugreports recently on the 3.3 
branch where reload generates moves to read-only memory.  The only way to 
prevent that without rewriting the compiler seems to be testing the /u flag.

> I think that simply making this flag a tri-state will be the cleanest fix.
> For memory to which this clearing optimization applies, the flag should not
> be set, because the memory is written twice.  If we're worried about
> pessimization in that case, we should avoid writing the memory twice.

I don't think this is radical enough.  I think we need to separate 
correctness from optimization, i.e having a way to say "this place can never 
ever be written to" and to say "this place is not supposed to have been 
written more than once because of the semantics of the language".  The 
latter could be relaxed by the middle-end if it deems it profitable, the 
former being of course immutable.

> Frankly, I suspect that there is virtually no real code where writing
> only to the holes (where "holes" means "fields that are not explicitly
> initialized to a non-zero value, and, if the compiler so desires, parts
> of the object that are not part of any field") has any observable
> performance from clearing the whole structure.  If most of the structure
> is zero, then that will certainly be true.  If only a tiny bit of the
> structure is non-zero, that will certainly be true.  If the non-zero
> parts are contiguous, that will probably be true.  In practice, there
> are few inner loops involving initializing every other field in a
> structure, and that is the case where we would lose.

Frankly, I don't feel confident enough to implement myself that change on the 
3.4 branch at this point.  On the other hand, it's clearly a better solution 
than the blockage.  So, if someone more confident than me wants to do it,  
I'll help him to merge its work with the patch from ACT and verify that it 
fixes all the known failures.


 2004-03-20  Eric Botcazou <ebotcazou@libertysurf.fr>
             Mark Mitchell <mark@codesourcery.com>

        PR optimization/13424
	* explow.c (maybe_set_unchanging): Revert 2003-04-07 patch.
	* tree.h (readwrite_fields_p): New prototype.
	* alias.c (readwrite_fields_p): New function.
        * expr.c (store_constructor): When clearing the aggregate because of
	an incomplete or mostly zero constructor, do not put the /u flag if the
	target is already unchanging.  Record whether a non-unchanging
	aggregate containing read-write fields is cleared with the /u flag.
	In that case, emit a blockage right after the clearing.


-- 
Eric Botcazou
Index: explow.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/explow.c,v
retrieving revision 1.118
diff -u -p -r1.118 explow.c
--- explow.c	13 Dec 2003 04:11:20 -0000	1.118
+++ explow.c	19 Mar 2004 19:18:45 -0000
@@ -597,18 +597,9 @@ maybe_set_unchanging (rtx ref, tree t)
   /* We can set RTX_UNCHANGING_P from TREE_READONLY for decls whose
      initialization is only executed once, or whose initializer always
      has the same value.  Currently we simplify this to PARM_DECLs in the
-     first case, and decls with TREE_CONSTANT initializers in the second.
-
-     We cannot do this for non-static aggregates, because of the double
-     writes that can be generated by store_constructor, depending on the
-     contents of the initializer.  Yes, this does eliminate a good fraction
-     of the number of uses of RTX_UNCHANGING_P for a language like Ada.
-     It also eliminates a good quantity of bugs.  Let this be incentive to
-     eliminate RTX_UNCHANGING_P entirely in favor of a more reliable
-     solution, perhaps based on alias sets.  */
+     first case, and decls with TREE_CONSTANT initializers in the second.  */
 
   if ((TREE_READONLY (t) && DECL_P (t)
-       && (TREE_STATIC (t) || ! AGGREGATE_TYPE_P (TREE_TYPE (t)))
        && (TREE_CODE (t) == PARM_DECL
 	   || (DECL_INITIAL (t) && TREE_CONSTANT (DECL_INITIAL (t)))))
       || TREE_CODE_CLASS (TREE_CODE (t)) == 'c')
Index: alias.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/alias.c,v
retrieving revision 1.209.2.4
diff -u -p -r1.209.2.4 alias.c
--- alias.c	12 Feb 2004 23:28:27 -0000	1.209.2.4
+++ alias.c	19 Mar 2004 19:18:53 -0000
@@ -312,6 +312,26 @@ readonly_fields_p (tree type)
 
   return 0;
 }
+
+/* Same as above but for read-write fields.  */
+
+int
+readwrite_fields_p (tree type)
+{
+  tree field;
+
+  if (TREE_CODE (type) != RECORD_TYPE && TREE_CODE (type) != UNION_TYPE
+      && TREE_CODE (type) != QUAL_UNION_TYPE)
+    return 0;
+
+  for (field = TYPE_FIELDS (type); field != 0; field = TREE_CHAIN (field))
+    if (TREE_CODE (field) == FIELD_DECL
+	&& (! TREE_READONLY (field)
+	    || readwrite_fields_p (TREE_TYPE (field))))
+      return 1;
+
+  return 0;
+}
 
 /* Return 1 if any MEM object of type T1 will always conflict (using the
    dependency routines in this file) with any MEM object of type T2.
Index: expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.615.4.9
diff -u -p -r1.615.4.9 expr.c
--- expr.c	13 Mar 2004 18:26:23 -0000	1.615.4.9
+++ expr.c	19 Mar 2004 19:19:19 -0000
@@ -4513,6 +4513,7 @@ store_constructor (tree exp, rtx target,
   if (TREE_CODE (type) == RECORD_TYPE || TREE_CODE (type) == UNION_TYPE
       || TREE_CODE (type) == QUAL_UNION_TYPE)
     {
+      bool readwrite_fields_marked_unchanging_p = false;
       tree elt;
 
       /* If size is zero or the target is already cleared, do nothing.  */
@@ -4552,15 +4553,28 @@ store_constructor (tree exp, rtx target,
 	{
 	  rtx xtarget = target;
 
-	  if (readonly_fields_p (type))
+	  if (readonly_fields_p (type) && ! RTX_UNCHANGING_P (xtarget))
 	    {
 	      xtarget = copy_rtx (xtarget);
 	      RTX_UNCHANGING_P (xtarget) = 1;
+	      if (readwrite_fields_p (type))
+		readwrite_fields_marked_unchanging_p = true;
 	    }
 
 	  clear_storage (xtarget, GEN_INT (size));
 	  cleared = 1;
 	}
+
+      /* ??? Emit a blockage to prevent the scheduler from swapping the
+	 memory write issued above with the /u flag and memory writes
+	 that may be issued later without it.  Note that the clearing
+	 above cannot be simply disabled in the unsafe cases because
+	 the C front-end relies on it to implement the semantics of
+	 constructors for automatic objects.  However, not all machine
+	 descriptions define a blockage insn, so emit an ASM_INPUT to
+	 act as one. ?*/
+      if (readwrite_fields_marked_unchanging_p)
+	emit_insn (gen_rtx_ASM_INPUT (VOIDmode, ""));
 
       if (! cleared)
 	emit_insn (gen_rtx_CLOBBER (VOIDmode, target));
Index: tree.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree.h,v
retrieving revision 1.458.2.4
diff -u -p -r1.458.2.4 tree.h
--- tree.h	8 Feb 2004 01:52:43 -0000	1.458.2.4
+++ tree.h	19 Mar 2004 19:19:27 -0000
@@ -2828,6 +2828,7 @@ extern void record_component_aliases (tr
 extern HOST_WIDE_INT get_alias_set (tree);
 extern int alias_sets_conflict_p (HOST_WIDE_INT, HOST_WIDE_INT);
 extern int readonly_fields_p (tree);
+extern int readwrite_fields_p (tree);
 extern int objects_must_conflict_p (tree, tree);
 
 /* In tree.c */

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]