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

[Eric Botcazou <ebotcazou at libertysurf dot fr>]

> I'd particularly like to understand this RTX_UNCHANGING_P optimization
> issue.  We've bumped into this before.  I think we need to take the
> conservative approach, even if that is pessimizing in some case.
> Would someone who understands the details of this bug please summarize
> it, mail that to me, and also add that information to the appropriate
> PR?

The problem is related to the semantics of the /u flag on memory writes.  
Strictly speaking, the /u flag should be put only if the memory location is 
guaranteed to be ever written once.

In practice, though, there has been a more liberal use of this /u flag 
because putting it on several writes to the same memory location doesn't 
seem to cause any problem as long as every write to the said location 
carries the /u flag (because the memory writes then conflict).

store_constructor features an optimization: if the constructor is mostly 
zero, the whole structure is cleared first, even though a few members are set 
to something else.  Later, if some individual members are read-only and set 
to something else, additional individual writes are issued and the /u flag 
is naturally put on them.  This was the initial state on the 3.3 branch.

First problem: if the whole structure contains read-only fields, is wholly 
cleared and at least one read-only field is set to non-zero, we have a 
memory write without the /u flag followed by a memory write with the /u 
flag.  These don't conflict so may be swapped by the scheduler.  Jakub fixed 
that on the 3.3 branch by not putting the /u flag on the individual writes 
if the whole structure has been cleared:

before:

	  if (TREE_READONLY (field))
	    {
	      if (GET_CODE (to_rtx) == MEM)
		to_rtx = copy_rtx (to_rtx);

	      RTX_UNCHANGING_P (to_rtx) = 1;
	    }

after

	  /* If the constructor has been cleared, setting RTX_UNCHANGING_P
	     on the MEM might lead to scheduling the clearing after the
	     store.  */
	  if (TREE_READONLY (field) && !cleared)
	    {
	      if (GET_CODE (to_rtx) == MEM)
		to_rtx = copy_rtx (to_rtx);

	      RTX_UNCHANGING_P (to_rtx) = 1;
	    }

This is the current state of the 3.3 branch.


Second problem: expand_expr contains these lines:

case INDIRECT_REF:
...
	/* If we are writing to this object and its type is a record with
	   readonly fields, we must mark it as readonly so it will
	   conflict with readonly references to those fields.  */
	if (modifier == EXPAND_WRITE && readonly_fields_p (type))
	  RTX_UNCHANGING_P (temp) = 1;

So we may end up with a record that contains some read-only fields, has been 
wholly cleared and is indirectly assigned.  In this case again, we have a 
memory write without the /u flag followed by a memory write with the /u 
flag.  Richard Kenner fixed that on the 3.4 branch *before* Jakub's patch by 
putting the /u flag during the clearing optimization in store_constructor:

before

      /* If the constructor has fewer fields than the structure
	 or if we are initializing the structure to mostly zeros,
	 clear the whole structure first.  Don't do this if TARGET is a
	 register whose mode size isn't equal to SIZE since clear_storage
	 can't handle this case.  */
      else if (((list_length (CONSTRUCTOR_ELTS (exp)) != fields_length 
(type))
		|| mostly_zeros_p (exp))
	       && (GET_CODE (target) != REG
		   || ((HOST_WIDE_INT) GET_MODE_SIZE (GET_MODE (target))
		       == size)))
	{
	  clear_storage (target, GEN_INT (size));
	  cleared = 1;
	}

after

      /* If the constructor has fewer fields than the structure
	 or if we are initializing the structure to mostly zeros,
	 clear the whole structure first.  Don't do this if TARGET is a
	 register whose mode size isn't equal to SIZE since clear_storage
	 can't handle this case.  */
      else if (((list_length (CONSTRUCTOR_ELTS (exp)) != fields_length 
(type))
		|| mostly_zeros_p (exp))
	       && (GET_CODE (target) != REG
		   || ((HOST_WIDE_INT) GET_MODE_SIZE (GET_MODE (target))
		       == size)))
	{
	  rtx xtarget = target;

	  if (readonly_fields_p (type))
	    {
	      xtarget = copy_rtx (xtarget);
	      RTX_UNCHANGING_P (xtarget) = 1;
	    }

	  clear_storage (xtarget, GEN_INT (size));
	  cleared = 1;
	}

This is the current state of the 3.4 branch and means that Jakub's patch is 
not valid on this branch (because otherwise we may have a memory write with 
the /u flag followed by a memory write without the /u flag for read-only 
fields).


Third problem: now, on the 3.4 branch, if the whole structure contains 
read-only fields and read-write fields, is wholly cleared and at least one 
read-write field is set to non-zero, we have a memory write with the /u flag 
followed by a memory write without the /u flag.  These of course don't 
conflict so may be swapped by the scheduler (this is my testcase on 
UltraSPARC for PR opt/13424).


A solution (by Olivier Hainque, that is now in the ACT tree) is to disable 
the clearing optimization in the unsafe cases, that is when we know that 
another write may be issued later which will not conflict:

      /* If the constructor has fewer fields than the structure or if
	 we are initializing the structure to mostly zeros, clear the
	 whole structure first. This is an optimization which avoids
	 having to clear several parts individually later on.  */
      else if (! cleared && size > 0
	       && ((list_length (CONSTRUCTOR_ELTS (exp))
		    != fields_length (type))
		   || mostly_zeros_p (exp))
	       /* Don't do this if TARGET is a register whose mode
  	          size isn't equal to SIZE since clear_storage can't
  	          handle this case.  */
	       && (GET_CODE (target) != REG
		   || ((HOST_WIDE_INT) GET_MODE_SIZE (GET_MODE (target))
		       == size))
	       /* Don't do this if the type has readonly fields or target is
		  marked unchanging. We would first end up with multiple
		  stores to const mem (to clear and then to initialize), and
		  this could also interfere with later stores to non readonly
		  fields.  */
	       && ! (readonly_fields_p (type) || RTX_UNCHANGING_P (target)))
	{
	  clear_storage (target, GEN_INT (size));
	  cleared = 1;
	}

Of course this pessimizes.  In particular, I think we could still clear if 
the whole structure is read-only, assuming we endorse the liberal use of the 
/u flag I was talking about above.

-- 
Eric Botcazou