Patchwork remove wrong code in immed_double_const

login
register
mail settings
Submitter Mike Stump
Date March 21, 2012, 1 a.m.
Message ID <B5BB677F-55F7-41F3-92A6-60B1D9577426@comcast.net>
Download mbox | patch
Permalink /patch/147889/
State New
Headers show

Comments

Mike Stump - March 21, 2012, 1 a.m.
On Mar 20, 2012, at 5:26 AM, Richard Sandiford wrote:
> So what I was trying to say was that if we remove the assert
> altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs,
> we need to define what the "implicit" high-order HWIs of a
> CONST_DOUBLE are, just like we already do for CONST_INT.

Now, since you expressed a preference for sign extending, and a worry that there might be new bugs exposed in the handling of CONST_DOUBLEs in the face of my change, I went through all the code again and tried my best to fix every other bug in the compiler at all related to this area that I could find ; that patch is below.  In this one, I updated the spec for CONST_DOUBLE to be sign extending.

Curious, plus_constant is just terribly broken in this are, now fixed.  mode_signbit_p is speced in English, so, I didn't want to misread or misunderstand it and swizzle it, so I left it alone for now.   Someone will have to describe what it does and I can try my hand at fixing it, if broken, I suspect it is.  As for simplify_const_unary_operation, I don't know what they were thinking, return 0 seems safer to me.

If there is any other code that I missed that people know about, I'd be happy to fix it, just let me know what code.  I did a pass on all the ports as well, and they seem reasonably clean about it.  The biggest problem is OImode -1, would come out as hex digits, and all the upper 0xfffff digits implied by sign extension would be missing.  A port that cared about OImode, trivially, would fix their output routine.  The debugging code has similar problems.

Is this closer to something you think is in the right direction?  If so, let figure out the right solution for mode_signbit_p and proceed from there.



@@ -1227,7 +1228,7 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
 	    return 0;
 	}
       else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
+	return 0;
       else
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
@@ -4987,6 +4988,7 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 	case CONST_DOUBLE:
 	  if (GET_MODE (el) == VOIDmode)
 	    {
+	      unsigned char extend = 0;
 	      /* If this triggers, someone should have generated a
 		 CONST_INT instead.  */
 	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
@@ -4999,13 +5001,15 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
 		  i += value_bit;
 		}
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  else
 	    {
+	      unsigned char extend = 0;
 	      long tmp[max_bitsize / 32];
 	      int bitsize = GET_MODE_BITSIZE (GET_MODE (el));
 
@@ -5030,10 +5034,10 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 		  *vp++ = tmp[ibase / 32] >> i % 32;
 		}
 
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  break;
Richard Sandiford - March 21, 2012, 1:17 p.m.
Mike Stump <mikestump@comcast.net> writes:
> diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
> index de45a22..0c6dc45 100644
> --- a/gcc/doc/rtl.texi
> +++ b/gcc/doc/rtl.texi
> @@ -1530,7 +1530,9 @@ Represents either a floating-point constant of mode @var{m} or an
>  integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
>  bits but small enough to fit within twice that number of bits (GCC
>  does not provide a mechanism to represent even larger constants).  In
> -the latter case, @var{m} will be @code{VOIDmode}.
> +the latter case, @var{m} will be @code{VOIDmode}.  For integral values
> +the value is a signed value, meaning the top bit of
> +@code{CONST_DOUBLE_HIGH} is a sign bit.
>  
>  @findex CONST_DOUBLE_LOW
>  If @var{m} is @code{VOIDmode}, the bits of the value are stored in

Sounds good.

> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 78ddfc3..c0b24e4 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -531,10 +531,9 @@ immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
>  
>       1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
>  	gen_int_mode.
> -     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
> -	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
> -	from copies of the sign bit, and sign of i0 and i1 are the same),  then
> -	we return a CONST_INT for i0.
> +     2) If the value of the integer fits into HOST_WIDE_INT anyway
> +        (i.e., i1 consists only from copies of the sign bit, and sign
> +	of i0 and i1 are the same), then we return a CONST_INT for i0.
>       3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
>    if (mode != VOIDmode)
>      {

This too.

> diff --git a/gcc/explow.c b/gcc/explow.c
> index 2fae1a1..6284d61 100644
> --- a/gcc/explow.c
> +++ b/gcc/explow.c
> @@ -96,6 +96,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)

For this I think we should make plus_constant a wrapper:

/* Return an rtx for the sum of X and the integer C.  */

rtx
plus_constant (rtx x, HOST_WIDE_INT c)
{
  return plus_constant_mode (GET_MODE (x), x, c);
}

/* Return an rtx for the sum of X and the integer C, given that X
   has mode MODE.  */

rtx
plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
{
  ...innards of current plus_constant, without the "mode = "...
}

Reason being...

>    switch (code)
>      {
>      case CONST_INT:
> +      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
> +	/* Punt for now.  */
> +	goto overflow;
>        return GEN_INT (INTVAL (x) + c);
>  
>      case CONST_DOUBLE:

...this won't work as things stand, since CONST_INT always has VOIDmode.
(I'm on a slow mission to fix that.)

I agree that this is a pre-existing bug.  Callers that want to be
CONST_INT-safe should use the new plus_constant_mode instead of
plus_constant.  Once they do, we should assert here that mode isn't
VOIDmode.  But since it's an existing bug that also affects 2-HWI
constants, I agree that replacing calls to plus_constant with calls
to plus_constant_mode is a separate fix.

I don't think it's a good idea to punt to a PLUS though.
(plus (const_int X) (const_int Y)) isn't canonical rtl,
and could cause other problems.

Suggest instead we reuse the CONST_DOUBLE code for CONST_INT,
with l1 set to INTVAL and h1 set to the sign extension.

> @@ -103,10 +106,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
>  	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
>  	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
>  	unsigned HOST_WIDE_INT l2 = c;
> -	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
> +	HOST_WIDE_INT h2 = c < 0 ? ~(HOST_WIDE_INT)0 : 0;
>  	unsigned HOST_WIDE_INT lv;
>  	HOST_WIDE_INT hv;
>  
> +	if (GET_MODE_BITSIZE (mode) > 2*HOST_WIDE_INT)
> +	  /* Punt for now.  */
> +	  goto overflow;
> +
>  	add_double (l1, h1, l2, h2, &lv, &hv);

Nicely, add_double returns true on overflow, so I think
we should replace the punt with:

   if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
     gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT);

(Seems better to explicitly specify the sign, even though
add_double would be equivalent.)

> @@ -141,6 +148,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)
>        break;
>  
>      case PLUS:
> +      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
> +	/* Punt for now.  */
> +	goto overflow;
>        /* The interesting case is adding the integer to a sum.
>  	 Look for constant term in the sum and combine
>  	 with C.  For an integer constant term, we make a combined

For this I think we should change the recursive CONSTANT_P call
to use plus_constant_mode (with the mode of the PLUS) instead of
plus_constant.  It will then be correct for CONST_INT, and we can
remove the special CONST_INT case.

> diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
> index ce4eab4..37e46b1 100644
> --- a/gcc/simplify-rtx.c
> +++ b/gcc/simplify-rtx.c
> @@ -101,6 +101,7 @@ mode_signbit_p (enum machine_mode mode, const_rtx x)
>  
>    if (width < HOST_BITS_PER_WIDE_INT)
>      val &= ((unsigned HOST_WIDE_INT) 1 << width) - 1;
> +  /* FIXME: audit for TImode and wider correctness.  */
>    return val == ((unsigned HOST_WIDE_INT) 1 << (width - 1));

We've already returned false in that case though.  I'm happy for this
function to be left as-is, but we could instead add a comment like:

    /* FIXME: We don't yet have a representation for wider modes.  */

above the:

    return false;

> @@ -1227,7 +1228,7 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
>  	    return 0;
>  	}
>        else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
> -	;
> +	return 0;
>        else
>  	hv = 0, lv &= GET_MODE_MASK (op_mode);

Should keep the current behaviour for == HOST_BITS_PER_WIDE_INT * 2.

I'd slightly prefer an assert rather than a "return false", but I won't
argue if another maintainer agrees with the return.

> @@ -4987,6 +4988,7 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
>  	case CONST_DOUBLE:
>  	  if (GET_MODE (el) == VOIDmode)
>  	    {
> +	      unsigned char extend = 0;
>  	      /* If this triggers, someone should have generated a
>  		 CONST_INT instead.  */
>  	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
> @@ -4999,13 +5001,15 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
>  		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
>  		  i += value_bit;
>  		}
> -	      /* It shouldn't matter what's done here, so fill it with
> -		 zero.  */
> +
> +	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
> +		extend = -1;
>  	      for (; i < elem_bitsize; i += value_bit)
> -		*vp++ = 0;
> +		*vp++ = extend;
>  	    }

Looks good.

>  	  else
>  	    {
> +	      unsigned char extend = 0;
>  	      long tmp[max_bitsize / 32];
>  	      int bitsize = GET_MODE_BITSIZE (GET_MODE (el));
>  
> @@ -5030,10 +5034,10 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
>  		  *vp++ = tmp[ibase / 32] >> i % 32;
>  		}
>  
> -	      /* It shouldn't matter what's done here, so fill it with
> -		 zero.  */
> +	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
> +		extend = -1;
>  	      for (; i < elem_bitsize; i += value_bit)
> -		*vp++ = 0;
> +		*vp++ = extend;
>  	    }

This is changing the real case, where sign extension doesn't make
much sense.

I think the expand_mult CONST_DOUBLE code needs changing too:

	  else if (CONST_DOUBLE_LOW (op1) == 0
		   && EXACT_POWER_OF_2_OR_ZERO_P (CONST_DOUBLE_HIGH (op1)))
	    {
	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
			  + HOST_BITS_PER_WIDE_INT;
	      return expand_shift (LSHIFT_EXPR, mode, op0,
				   shift, target, unsignedp);
	    }

This isn't correct if the mode is wider than 2 HWIs and
CONST_DOUBLE_HIGH has the high bit set.

Same for:

      /* Likewise for multipliers wider than a word.  */
      if (GET_CODE (trueop1) == CONST_DOUBLE
	  && (GET_MODE (trueop1) == VOIDmode
	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
	  && GET_MODE (op0) == mode
	  && CONST_DOUBLE_LOW (trueop1) == 0
	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
	return simplify_gen_binary (ASHIFT, mode, op0,
				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));

in the MULT case of simplify_binary_operation_1.

simplify_const_unary_operation needs to check for overflow
when handling 2-HWI arithmetic, since we can no longer assume
that the result is <= 2 HWIs in size.  E.g.:

	case NEG:
	  neg_double (l1, h1, &lv, &hv);
	  break;

should probably be:

	case NEG:
	  if (neg_double (l1, h1, &lv, &hv))
            gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT);
	  break;

and similarly for other cases.

Richard

Patch

diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
index de45a22..0c6dc45 100644
--- a/gcc/doc/rtl.texi
+++ b/gcc/doc/rtl.texi
@@ -1530,7 +1530,9 @@  Represents either a floating-point constant of mode @var{m} or an
 integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
 bits but small enough to fit within twice that number of bits (GCC
 does not provide a mechanism to represent even larger constants).  In
-the latter case, @var{m} will be @code{VOIDmode}.
+the latter case, @var{m} will be @code{VOIDmode}.  For integral values
+the value is a signed value, meaning the top bit of
+@code{CONST_DOUBLE_HIGH} is a sign bit.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 78ddfc3..c0b24e4 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -531,10 +531,9 @@  immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+        (i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
diff --git a/gcc/explow.c b/gcc/explow.c
index 2fae1a1..6284d61 100644
--- a/gcc/explow.c
+++ b/gcc/explow.c
@@ -96,6 +96,9 @@  plus_constant (rtx x, HOST_WIDE_INT c)
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
+	/* Punt for now.  */
+	goto overflow;
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -103,10 +106,14 @@  plus_constant (rtx x, HOST_WIDE_INT c)
 	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
 	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
 	unsigned HOST_WIDE_INT l2 = c;
-	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
+	HOST_WIDE_INT h2 = c < 0 ? ~(HOST_WIDE_INT)0 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
+	if (GET_MODE_BITSIZE (mode) > 2*HOST_WIDE_INT)
+	  /* Punt for now.  */
+	  goto overflow;
+
 	add_double (l1, h1, l2, h2, &lv, &hv);
 
 	return immed_double_const (lv, hv, VOIDmode);
@@ -141,6 +148,9 @@  plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
+      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
+	/* Punt for now.  */
+	goto overflow;
       /* The interesting case is adding the integer to a sum.
 	 Look for constant term in the sum and combine
 	 with C.  For an integer constant term, we make a combined
@@ -185,6 +195,7 @@  plus_constant (rtx x, HOST_WIDE_INT c)
       break;
     }
 
+ overflow:
   if (c != 0)
     x = gen_rtx_PLUS (mode, x, GEN_INT (c));
 
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index ce4eab4..37e46b1 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -101,6 +101,7 @@  mode_signbit_p (enum machine_mode mode, const_rtx x)
 
   if (width < HOST_BITS_PER_WIDE_INT)
     val &= ((unsigned HOST_WIDE_INT) 1 << width) - 1;
+  /* FIXME: audit for TImode and wider correctness.  */
   return val == ((unsigned HOST_WIDE_INT) 1 << (width - 1));
 }