diff mbox

remove wrong code in immed_double_const

Message ID 83448720-93D1-483D-8B8C-08B672399E1B@comcast.net
State New
Headers show

Commit Message

Mike Stump March 22, 2012, 8:27 p.m. UTC 
On Mar 22, 2012, at 3:16 AM, Richard Sandiford wrote:
> Sorry, meant we should leave the svn version as it is.

Ah, I see now, I agree, I removed that bit (extend in the floating point case) of my change from the patch.

> I think this should be s/is smaller than/is different from/.

Sounds good, fixed.

> Should be:
> 
> 	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)

As you noted, you do mean:

	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)

Fixed.

> Nitlet, but line is wider than 80 chars.  Probably easiest fix is:
> 
> 	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
> 	  tem = force_const_mem (GET_MODE (x), tem);

Fixed.

>> +	      if (shift < 2*HOST_BITS_PER_WIDE_INT-1
>> +		  || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT)
> 
> Missing spaces around binary operators.

Fixed all instances of 2*HOST and INT-1, there were many, not just this one.  Some pre-date my patch.

>> @@ -1219,15 +1220,10 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
>>       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_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
>> -	;
>> +      if (op_mode == VOIDmode
>> +	  || GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
>> +	/* We should never get a negative number.  */
>> +	gcc_assert (hv >= 0);
>>       else
>> 	hv = 0, lv &= GET_MODE_MASK (op_mode);
>> 
> 
> Sorry, with this bit, I meant that the current svn code is correct
> for GET_MODE_BITSIZE (op_mode) == HOST_BITS_PER_WIDE_INT * 2.
> In that case, hv < 0 can just mean that we have a uint128_t
> (or whatever) whose high bit happens to be set.

Well, according to the spec, one cannot use CONST_DOUBLE to represent a uint128 value with the high bit set.  The C frontend type plays this game, but they can, because they track the type with the constant the the values of the constant are interpreted exclusively in  the context of the type.  Since we don't have the unsigned bit, we can't, so, either, they are speced to be values on their own, or values dependent upon some external notion.  By changing the spec to say sign extending, we mean if the high bit is set, the value is negative.  If this is the wrong value for a uint value, then that representation cannot be used.  The only solution is to use a different representation (CONST_QUAD, CONST_UDOUBLE or something else). 

I'm thought about that routine some more, and it is just totally broken.  I've changed the code to:

      /* We should never get a negative number.  */
      gcc_assert (hv >= 0);

      if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT)
        hv = 0, lv &= GET_MODE_MASK (op_mode);

negative numbers are bad, period, this badness, doesn't depend upon anything other than the value being negative.  The use of hv = 0, can only be done with PRECISIONs less than or equal to HOST_BITS_PER_WIDE_INT.  Wider than this, and some of hv would be wiped out, which would be wrong.

Thoughts?

> OK with those changes as far as the RTL optimisations go.

Not quite.  Need to resolve the point just above.  But, yea, we are getting very close now.  I'm switched over to a patch for trunk (from 4.6.0) and added the ChangeLog.  The switch to GET_MODE_PRECISION above was one change, for example.  The rest fit in nicely, the arg to expand_shift was updated slightly to match trunk.

> I can't approve the immed_double_const change itself.  Sounds like Richard G would be
> willing though.

I've bundled in the patch that started this all into this version.  I'll look forward to his review.
* doc/rtl.texi (const_double): Document as sign-extending.
	* expmed.c (expand_mult): Ensure we don't use shift
	incorrectly.
	* emit-rtl.c (immed_double_int_const): Refine to state the
	value is signed.
	* simplify-rtx.c (mode_signbit_p): Add a fixme for wider than
	CONST_DOUBLE integers.
	(simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no
	negative values are converted.  Fix conversions bigger than
	HOST_BITS_PER_WIDE_INT.
	(simplify_binary_operation_1): Ensure we don't use shift
	incorrectly.
	(simplify_immed_subreg): Sign-extend CONST_DOUBLEs.
	* explow.c (plus_constant_mode): Add.
	(plus_constant): Implement with plus_constant_mode.
	* rtl.h (plus_constant_mode): Add.

Comments

Richard Sandiford March 23, 2012, 10:01 a.m. UTC | #1 
Mike Stump <mikestump@comcast.net> writes:
>> Sorry, with this bit, I meant that the current svn code is correct
>> for GET_MODE_BITSIZE (op_mode) == HOST_BITS_PER_WIDE_INT * 2.
>> In that case, hv < 0 can just mean that we have a uint128_t
>> (or whatever) whose high bit happens to be set.

(To be clear, I was using uint128_t as an example of a 2-HWI type,
assuming we're using 64-bit HWIs -- which I hope we are for targets
where this assert matters.)

> Well, according to the spec, one cannot use CONST_DOUBLE to represent
> a uint128 value with the high bit set.

We can!  And do now, even without your patch.  Because...

> The C frontend type plays this game, but they can, because they track
> the type with the constant the the values of the constant are
> interpreted exclusively in the context of the type.  Since we don't
> have the unsigned bit, we can't, so, either, they are speced to be
> values on their own, or values dependent upon some external notion.
> By changing the spec to say sign extending, we mean if the high bit is
> set, the value is negative.

...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).
For things like addition where signed vs. unsigned interpretation
doesn't matter, we have a single rtx op like PLUS.  For things like
multiplication where it does matter, we have separate signed and
unsigned variants.  There is nothing to distinguish a uint128_t
_register_ (i.e. TImode REG) that has the upper bit set from an
int128_t register that happens to be negative.  Instead the
interpretation is decided by the operation.  And the same principle
applies to constants.  There isn't, and doesn't need to be,
a separate CONST_DOUBLE representation for:

  - an unsigned 2-HWI value that has the upper bit set and
  - a signed 2-HWI value that is negative

The sign-extending thing is simply there to specify what happens when an
N>2 HWI value is represented as a 2-HWI rtx.  I.e. it's simply there to
say what the implicit N-2 HWIs are.  (That's why the definition only
matters now that we're allowing N>2 by removing the assert.)

In this context we're interpreting the value as unsigned because we have
an UNSIGNED_FLOAT operation.  So if the mode of the operand is exactly
2 HWIs in size, a negative high HWI simply indicates an unsigned value
that has the high bit set.

The same principle already applies to CONST_INT.  We have long defined
CONST_INT to be a sign-extending representation, in the sense that it
is allowed to represent 2-HWI modes in which the upper HWI happens
to be a sign extension of the lower HWI.  That doesn't mean the 2-HWI
constant itself is negative: it can just as easily be a high unsigned
value.  Whether it is signed, unsigned or neutral depends on the context
of the rtx operation.

All we're doing here is extending that principle to CONST_DOUBLE
and modes wider than 2 HWIs.

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

Richard
diff mbox

Patch

Index: doc/rtl.texi
===================================================================
--- doc/rtl.texi	(revision 185706)
+++ doc/rtl.texi	(working copy)
@@ -1510,7 +1510,9 @@  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
+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
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,7 +517,8 @@  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
+   The value is a signed value, with the high bit of i1 being the sign
+   bit.  Do not use this routine for non-integer modes; convert to
    REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
 
 rtx
@@ -531,10 +532,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 +546,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,10 @@  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
+      /* We should never get a negative number.  */
+      gcc_assert (hv >= 0);
+
+      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 +1713,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 +1778,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 +2375,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 +5186,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 +5199,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);