[PATCH] PR optimization/11059: zero length in clear_storage (take2)

[Roger Sayle <roger at eyesopen dot com>]

Hi Jim,

Very many thanks for your review.  The following patch addresses all
but one of the issues pointed out by your comments.  The one sticking
point is that I still think its reasonable to place safety checks in
these middle-end routines.  Admittedly, we're only currently aware of
just a single broken caller.  However, we cannot preclude that some
other callers could potentially also pass zero length, or that new users
of these functions will be added to GCC over time.  But, it didn't sound
like you were strongly opposed to the checks themselves.


> However, I find it contradictory that one of your safety checks adds an
> unchecked assumption that endp != 2, which means it isn't safe.  If we
> are adding safety checks, we should do it right.

Agreed.  In the revised patch below, we call abort if store_by_pieces
is ever called with zero length and endp is two.  Admittedly, I currently
know that all endp == 2 callers are safe, but as you point out, we need
to be consistent.


> It isn't clear why you mentioned that there was apparently a problem
> with the C++ front end constructor code.  I don't see anything obviously
> wrong there.  I think the difference here is due to C/C++ language
> differences.

My mistake.  I didn't appreciate that store_constructor was a middle-end
function when I was looking at the call stack when debugging the abort.
The C++ front end is fine, the problem is in store_constructor.


> Looking at store_constructor, I see that it already has a check for size
>  > 0 for structure/array types, just not for the union type.  So fixing
> clear_storage means that we now handle unions and structures differently
> in store_constructor, which does not seem right. In the former case we
> get a clobber for zero size structures, with your patch we would get
> nothing for zero size unions.  I'd suggest adding the missing size > 0
> check to store_constructor so that unions and structures are handled the
> same.

Another good catch.  Indeed the behavior of zero length unions and
structures should be identical, and for the test case in the PR, we can
prevent clear_storage being called with zero length by handling this
case in store_constructor.

However, I believe the correct solution is to consider a zero sized
structure already cleared and avoid emiting the clobber insn into
the RTL stream.  The clobber is used as a blockage/barrier when
initializing const structs/arrays that are mostly zero.  In this case,
we clear the entire array to zero, the set the non-zero elements.
However, this means that we first store a zero, then store a non-zero
value into a MEM marked as const. This really confuses alias analysis,
as the const mem can't alias with a write, and the zero store and the
non-zero store may end up being swapped.  To prevent this, GCC places
a scheduling barrier after the clear_storage to prevent problems.

Obviously, if the length of the structure or union is zero, there's
no need to emit the blockage, which avoids the serialization and
provides better scheduling opportunities.  If nothing else we avoid
emitting an additional insn, which should use less memory and speed
things up a teeny bit.

As an additional benefit, treating "size == 0 || cleared" at the start
of this if-then-else chain also simplifies much of the following logic.



The revised patch below has been tested on i686-pc-linux-gnu with a
complete "make boostrap", all languages except treelang, and regression
tested with a top-level "make -k check" with no new failures.

Ok for mainline?


2003-07-04  Roger Sayle  <roger@eyesopen.com>

	PR optimization/11059
	* expr.c (can_store_by_pieces): Return true if length is zero.
	(store_by_pieces): If length is zero and endp is two, abort,
	othwerise, if length is zero and endp is not two, return "to".
	(clear_by_pieces): Do nothing if length is zero.
	(clear_storage): Do nothing if length is zero.
	(store_constructor): Simplify code when size is zero, or the
	target has already been cleared.  This avoids emitting a
	blockage instruction when initializing empty structures.

	* g++.dg/opt/emptyunion.C: New testcase.


Index: expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.555
diff -c -3 -p -r1.555 expr.c
*** expr.c	23 Jun 2003 15:27:35 -0000	1.555
--- expr.c	4 Jul 2003 15:09:20 -0000
*************** can_store_by_pieces (len, constfun, cons
*** 2729,2734 ****
--- 2729,2737 ----
    int reverse;
    rtx cst;

+   if (len == 0)
+     return 1;
+
    if (! STORE_BY_PIECES_P (len, align))
      return 0;

*************** store_by_pieces (to, len, constfun, cons
*** 2808,2813 ****
--- 2811,2823 ----
  {
    struct store_by_pieces data;

+   if (len == 0)
+     {
+       if (endp == 2)
+ 	abort ();
+       return to;
+     }
+
    if (! STORE_BY_PIECES_P (len, align))
      abort ();
    to = protect_from_queue (to, 1);
*************** clear_by_pieces (to, len, align)
*** 2859,2864 ****
--- 2869,2877 ----
  {
    struct store_by_pieces data;

+   if (len == 0)
+     return;
+
    data.constfun = clear_by_pieces_1;
    data.constfundata = NULL;
    data.len = len;
*************** clear_storage (object, size)
*** 3029,3035 ****
        object = protect_from_queue (object, 1);
        size = protect_from_queue (size, 0);

!       if (GET_CODE (size) == CONST_INT
  	  && CLEAR_BY_PIECES_P (INTVAL (size), align))
  	clear_by_pieces (object, INTVAL (size), align);
        else if (clear_storage_via_clrstr (object, size, align))
--- 3042,3050 ----
        object = protect_from_queue (object, 1);
        size = protect_from_queue (size, 0);

!       if (GET_CODE (size) == CONST_INT && INTVAL (size) == 0)
! 	;
!       else if (GET_CODE (size) == CONST_INT
  	  && CLEAR_BY_PIECES_P (INTVAL (size), align))
  	clear_by_pieces (object, INTVAL (size), align);
        else if (clear_storage_via_clrstr (object, size, align))
*************** store_constructor (exp, target, cleared,
*** 4971,4981 ****
      {
        tree elt;

        /* We either clear the aggregate or indicate the value is dead.  */
!       if ((TREE_CODE (type) == UNION_TYPE
! 	   || TREE_CODE (type) == QUAL_UNION_TYPE)
! 	  && ! cleared
! 	  && ! CONSTRUCTOR_ELTS (exp))
  	/* If the constructor is empty, clear the union.  */
  	{
  	  clear_storage (target, expr_size (exp));
--- 4986,4998 ----
      {
        tree elt;

+       /* If size is zero or the target is already cleared, do nothing.  */
+       if (size == 0 || cleared)
+ 	cleared = 1;
        /* We either clear the aggregate or indicate the value is dead.  */
!       else if ((TREE_CODE (type) == UNION_TYPE
! 		|| TREE_CODE (type) == QUAL_UNION_TYPE)
! 	       && ! CONSTRUCTOR_ELTS (exp))
  	/* If the constructor is empty, clear the union.  */
  	{
  	  clear_storage (target, expr_size (exp));
*************** store_constructor (exp, target, cleared,
*** 4986,4992 ****
  	 set the initial value as zero so we can fold the value into
  	 a constant.  But if more than one register is involved,
  	 this probably loses.  */
!       else if (! cleared && GET_CODE (target) == REG && TREE_STATIC (exp)
  	       && GET_MODE_SIZE (GET_MODE (target)) <= UNITS_PER_WORD)
  	{
  	  emit_move_insn (target, CONST0_RTX (GET_MODE (target)));
--- 5003,5009 ----
  	 set the initial value as zero so we can fold the value into
  	 a constant.  But if more than one register is involved,
  	 this probably loses.  */
!       else if (GET_CODE (target) == REG && TREE_STATIC (exp)
  	       && GET_MODE_SIZE (GET_MODE (target)) <= UNITS_PER_WORD)
  	{
  	  emit_move_insn (target, CONST0_RTX (GET_MODE (target)));
*************** store_constructor (exp, target, cleared,
*** 4998,5007 ****
  	 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 (! cleared && size > 0
! 	       && ((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)))
--- 5015,5022 ----
  	 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)))



// PR optimization/11059
// This testcase ICEd because clear_by_pieces was called with zero length.
// { dg-do compile }
// { dg-options "-O2" }

union uni {};

int main() {
  uni *h;

  h = (uni *)new uni();
}


Roger
--