diff mbox

remove wrong code in immed_double_const

Message ID CA9A8137-1C25-45E8-AB2C-3C2D3B1F39F9@comcast.net
State New
Headers show

Commit Message

Mike Stump March 26, 2012, 7:14 p.m. UTC
On Mar 23, 2012, at 3:01 AM, Richard Sandiford wrote:
> ...it doesn't mean that we interpret the value as a negative _rtx_.
> As with all rtx calculations, things like signedness and saturation are
> decided by the operation rather than the "type" ("type" == rtx mode).

Ah...  [ light goes on ]  Let me adjust the documentation to be exceptionally clear in this case.  Check out the new wording on const_int, const_double and on immed_double_const.  I fixed simplify_const_unary_operation to match your suggestion.

> Sorry for the rather rambling explanation :-)  I still think the
> version I suggested for this hunk is right though.

I agree.  I now see what I had wrong.  Thanks for your patience and explanations.  If you like the wording I used in the doc and on immed_double_const, I think we have now resolved all issues.  The previous version was bootstraped and regression tested on darwin, fortran, c, c++, objective-c++...  I'll do one more run with the update for simplify_const_unary_operation, as that can trip when before it would merely return 0.

Ok?

Comments

Richard Sandiford March 26, 2012, 8:03 p.m. UTC | #1
Mike Stump <mikestump@comcast.net> writes:
> On Mar 23, 2012, at 3:01 AM, Richard Sandiford wrote:
>> ...it doesn't mean that we interpret the value as a negative _rtx_.
>> As with all rtx calculations, things like signedness and saturation are
>> decided by the operation rather than the "type" ("type" == rtx mode).
>
> Ah...  [ light goes on ]  Let me adjust the documentation to be exceptionally clear in this case.  Check out the new wording on const_int, const_double and on immed_double_const.  I fixed simplify_const_unary_operation to match your suggestion.
>
>> Sorry for the rather rambling explanation :-)  I still think the
>> version I suggested for this hunk is right though.
>
> I agree.  I now see what I had wrong.  Thanks for your patience and explanations.  If you like the wording I used in the doc and on immed_double_const, I think we have now resolved all issues.  The previous version was bootstraped and regression tested on darwin, fortran, c, c++, objective-c++...  I'll do one more run with the update for simplify_const_unary_operation, as that can trip when before it would merely return 0.
>
> Ok?
>
>
> Index: doc/rtl.texi
> ===================================================================
> --- doc/rtl.texi	(revision 185706)
> +++ doc/rtl.texi	(working copy)
> @@ -1479,8 +1479,13 @@ This type of expression represents the i
>  is customarily accessed with the macro @code{INTVAL} as in
>  @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}.
>  
> -Constants generated for modes with fewer bits than @code{HOST_WIDE_INT}
> -must be sign extended to full width (e.g., with @code{gen_int_mode}).
> +Constants generated for modes with fewer bits than in
> +@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with
> +@code{gen_int_mode}).  For constants for modes with more bits than in
> +@code{HOST_WIDE_INT} the implied high order bits of that constant are
> +copies of the top bit, however values are neither signed nor unsigned.
> +All operations defined on such constants define the signededness.

I'm not someone who should be wordsmithing, but I think:

...copies of the top bit.  Note however that values are neither inherently
signed nor inherently unsigned; where necessary, signedness is determined
by the rtl operation instead.

> @@ -1510,7 +1515,12 @@ Represents either a floating-point const
>  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
> +constants for modes with more bits than twice the number in
> +@code{HOST_WIDE_INT} the implied high order bits of that constant are
> +copies of the top bit of @code{CONST_DOUBLE_HIGH}, however integral
> +values are neither signed nor unsigned.  All operations defined on
> +such constants define the signededness.

Same idea here.

> +/* Return an rtx for the sum of X and the integer C, given that X has
> +   mode MODE.  This routine should be used instead of plus_constant
> +   when they want to ensure that addition happens in a particular
> +   mode, which is necessary when x can be a VOIDmode CONST_INT or

Sorry, just noticed, should be "...when X can be..."

Looks good to me otherwise, thanks.  Richi?

Richard
diff mbox

Patch

Index: doc/rtl.texi
===================================================================
--- doc/rtl.texi	(revision 185706)
+++ doc/rtl.texi	(working copy)
@@ -1479,8 +1479,13 @@  This type of expression represents the i
 is customarily accessed with the macro @code{INTVAL} as in
 @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}.
 
-Constants generated for modes with fewer bits than @code{HOST_WIDE_INT}
-must be sign extended to full width (e.g., with @code{gen_int_mode}).
+Constants generated for modes with fewer bits than in
+@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with
+@code{gen_int_mode}).  For constants for modes with more bits than in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit, however values are neither signed nor unsigned.
+All operations defined on such constants define the signededness.
+
 
 @findex const0_rtx
 @findex const1_rtx
@@ -1510,7 +1515,12 @@  Represents either a floating-point const
 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
+constants for modes with more bits than twice the number in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit of @code{CONST_DOUBLE_HIGH}, however integral
+values are neither signed nor unsigned.  All operations defined on
+such constants define the signededness.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
Index: expmed.c
===================================================================
--- expmed.c	(revision 185706)
+++ expmed.c	(working copy)
@@ -3135,8 +3135,10 @@  expand_mult (enum machine_mode mode, rtx
 	    {
 	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
 			  + HOST_BITS_PER_WIDE_INT;
-	      return expand_shift (LSHIFT_EXPR, mode, op0,
-				   shift, target, unsignedp);
+	      if (shift < 2 * HOST_BITS_PER_WIDE_INT - 1
+		  || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)
+		return expand_shift (LSHIFT_EXPR, mode, op0,
+				     shift, target, unsignedp);
 	    }
 	}
 
Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 185706)
+++ emit-rtl.c	(working copy)
@@ -517,8 +517,11 @@  immed_double_int_const (double_int i, en
 
 /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
    of ints: I0 is the low-order word and I1 is the high-order word.
-   Do not use this routine for non-integer modes; convert to
-   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
+   For values that are larger than 2*HOST_BITS_PER_WIDE_INT, the
+   implied upper bits are copies of the high bit of i1.  The value
+   itself is neither signed nor unsigned.  Do not use this routine for
+   non-integer modes; convert to REAL_VALUE_TYPE and use
+   CONST_DOUBLE_FROM_REAL_VALUE.  */
 
 rtx
 immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
@@ -531,10 +534,9 @@  immed_double_const (HOST_WIDE_INT i0, HO
 
      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)
     {
@@ -546,8 +548,6 @@  immed_double_const (HOST_WIDE_INT i0, HO
 
       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
 	return gen_int_mode (i0, mode);
-
-      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
     }
 
   /* If this integer fits in one word, return a CONST_INT.  */
Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 185706)
+++ simplify-rtx.c	(working copy)
@@ -97,6 +97,7 @@  mode_signbit_p (enum machine_mode mode, 
       width -= HOST_BITS_PER_WIDE_INT;
     }
   else
+    /* FIXME: We don't yet have a representation for wider modes.  */
     return false;
 
   if (width < HOST_BITS_PER_WIDE_INT)
@@ -1355,16 +1356,11 @@  simplify_const_unary_operation (enum rtx
       else
 	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
 
-      if (op_mode == VOIDmode)
-	{
-	  /* We don't know how to interpret negative-looking numbers in
-	     this case, so don't try to fold those.  */
-	  if (hv < 0)
-	    return 0;
-	}
-      else if (GET_MODE_PRECISION (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
-      else
+      if (op_mode == VOIDmode
+	  || GET_MODE_PRECISION (op_mode) > 2 * HOST_BITS_PER_WIDE_INT)
+	/* We should never get a negative number.  */
+	gcc_assert (hv >= 0);
+      else if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT)
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
       REAL_VALUE_FROM_UNSIGNED_INT (d, lv, hv, mode);
@@ -1718,7 +1714,7 @@  simplify_const_unary_operation (enum rtx
   else if (GET_CODE (op) == CONST_DOUBLE
 	   && SCALAR_FLOAT_MODE_P (GET_MODE (op))
 	   && GET_MODE_CLASS (mode) == MODE_INT
-	   && width <= 2*HOST_BITS_PER_WIDE_INT && width > 0)
+	   && width <= 2 * HOST_BITS_PER_WIDE_INT && width > 0)
     {
       /* Although the overflow semantics of RTL's FIX and UNSIGNED_FIX
 	 operators are intentionally left unspecified (to ease implementation
@@ -1783,7 +1779,7 @@  simplify_const_unary_operation (enum rtx
 	    return const0_rtx;
 
 	  /* Test against the unsigned upper bound.  */
-	  if (width == 2*HOST_BITS_PER_WIDE_INT)
+	  if (width == 2 * HOST_BITS_PER_WIDE_INT)
 	    {
 	      th = -1;
 	      tl = -1;
@@ -2380,7 +2376,9 @@  simplify_binary_operation_1 (enum rtx_co
 	      || 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)
+	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0
+	  && (val < 2 * HOST_BITS_PER_WIDE_INT - 1
+	      || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT))
 	return simplify_gen_binary (ASHIFT, mode, op0,
 				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));
 
@@ -5189,6 +5187,7 @@  simplify_immed_subreg (enum machine_mode
 	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);
@@ -5201,10 +5200,11 @@  simplify_immed_subreg (enum machine_mode
 		    = 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
 	    {
Index: explow.c
===================================================================
--- explow.c	(revision 185706)
+++ explow.c	(working copy)
@@ -74,14 +74,20 @@  trunc_int_for_mode (HOST_WIDE_INT c, enu
   return c;
 }
 
-/* Return an rtx for the sum of X and the integer C.  */
+/* Return an rtx for the sum of X and the integer C, given that X has
+   mode MODE.  This routine should be used instead of plus_constant
+   when they want to ensure that addition happens in a particular
+   mode, which is necessary when x can be a VOIDmode CONST_INT or
+   CONST_DOUBLE and the width of the constant is different from the
+   width of the expression.  */
+/* TODO: All callers of plus_constant should migrate to this routine,
+   and once they do, we can assert that mode is not VOIDmode.  */
 
 rtx
-plus_constant (rtx x, HOST_WIDE_INT c)
+plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
 {
   RTX_CODE code;
   rtx y;
-  enum machine_mode mode;
   rtx tem;
   int all_constant = 0;
 
@@ -91,12 +97,26 @@  plus_constant (rtx x, HOST_WIDE_INT c)
  restart:
 
   code = GET_CODE (x);
-  mode = GET_MODE (x);
   y = x;
 
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
+	{
+	  unsigned HOST_WIDE_INT l1 = INTVAL (x);
+	  HOST_WIDE_INT h1 = (l1 >> (HOST_BITS_PER_WIDE_INT - 1)) ? -1 : 0;
+	  unsigned HOST_WIDE_INT l2 = c;
+	  HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
+	  unsigned HOST_WIDE_INT lv;
+	  HOST_WIDE_INT hv;
+
+	  if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	    gcc_unreachable ();
+
+	  return immed_double_const (lv, hv, VOIDmode);
+	}
+
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -104,11 +124,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 ? -1 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
-	add_double (l1, h1, l2, h2, &lv, &hv);
+	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	  /* Sorry, we have no way to represent overflows this wide.
+	     To fix, add constant support wider than CONST_DOUBLE.  */
+	  gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT);
 
 	return immed_double_const (lv, hv, VOIDmode);
       }
@@ -120,10 +143,8 @@  plus_constant (rtx x, HOST_WIDE_INT c)
       if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
 	  && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))
 	{
-	  tem
-	    = force_const_mem (GET_MODE (x),
-			       plus_constant (get_pool_constant (XEXP (x, 0)),
-					      c));
+	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
+	  tem = force_const_mem (GET_MODE (x), tem);
 	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
 	    return tem;
 	}
@@ -142,31 +163,17 @@  plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
-      /* 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
-	 integer.  For a constant term that is not an explicit integer,
-	 we cannot really combine, but group them together anyway.
-
-	 Restart or use a recursive call in case the remaining operand is
-	 something that we handle specially, such as a SYMBOL_REF.
+      /* 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 or a constant term that is not an
+	 explicit integer, we combine or group them together anyway.
 
 	 We may not immediately return from the recursive call here, lest
 	 all_constant gets lost.  */
 
-      if (CONST_INT_P (XEXP (x, 1)))
+      if (CONSTANT_P (XEXP (x, 1)))
 	{
-	  c += INTVAL (XEXP (x, 1));
-
-	  if (GET_MODE (x) != VOIDmode)
-	    c = trunc_int_for_mode (c, GET_MODE (x));
-
-	  x = XEXP (x, 0);
-	  goto restart;
-	}
-      else if (CONSTANT_P (XEXP (x, 1)))
-	{
-	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant (XEXP (x, 1), c));
+	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant_mode (mode, XEXP (x, 1), c));
 	  c = 0;
 	}
       else if (find_constant_term_loc (&y))
@@ -176,7 +183,7 @@  plus_constant (rtx x, HOST_WIDE_INT c)
 	  rtx copy = copy_rtx (x);
 	  rtx *const_loc = find_constant_term_loc (&copy);
 
-	  *const_loc = plus_constant (*const_loc, c);
+	  *const_loc = plus_constant_mode (mode, *const_loc, c);
 	  x = copy;
 	  c = 0;
 	}
@@ -196,6 +203,14 @@  plus_constant (rtx x, HOST_WIDE_INT c)
   else
     return x;
 }
+
+/* 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);
+}
 
 /* If X is a sum, return a new sum like X but lacking any constant terms.
    Add all the removed constant terms into *CONSTPTR.
Index: rtl.h
===================================================================
--- rtl.h	(revision 185706)
+++ rtl.h	(working copy)
@@ -1644,6 +1644,7 @@  extern int ceil_log2 (unsigned HOST_WIDE
 /* In explow.c */
 extern HOST_WIDE_INT trunc_int_for_mode	(HOST_WIDE_INT, enum machine_mode);
 extern rtx plus_constant (rtx, HOST_WIDE_INT);
+extern rtx plus_constant_mode (enum machine_mode, rtx, HOST_WIDE_INT);
 
 /* In rtl.c */
 extern rtx rtx_alloc_stat (RTX_CODE MEM_STAT_DECL);