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.