Re: remove wrong code in immed_double_const

[Richard Sandiford <rdsandiford@googlemail.com>]

Mike Stump <mikestump@comcast.net> writes:
>> This is changing the real case, where sign extension doesn't make
>> much sense.
>
> I'm not sure I followed.  Do you want me to remove the change for the
> second case, leave it as it, or something else?  I've left it as I had
> modified it.

Sorry, meant we should leave the svn version as it is.  The new docs
(rightly) only mention sign-extension for integer modes, so the comment
about it not mattering for FP modes is still correct.  (In principle
I'd prefer to replace it with an assert, but that's a separate change.
Nothing we're doing here should change whether the FP path gets executed.)

>> 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;
>
> Are you talking about the block of code inside:
>
>   /* We can do some operations on integer CONST_DOUBLEs.  Also allow                                                                              
>      for a DImode operation on a CONST_INT.  */
>   else if (GET_MODE (op) == VOIDmode
>            && width <= HOST_BITS_PER_WIDE_INT * 2
>
> ?

Heh. it seems so :-)  Never mind that bit then.

> diff --git a/gcc/explow.c b/gcc/explow.c
> index 2fae1a1..c94ad25 100644
> --- a/gcc/explow.c
> +++ b/gcc/explow.c
> @@ -73,14 +73,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enum machine_mode mode)
>    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 smaller than the
> +   width of the expression.  */

I think this should be s/is smaller than/is different from/.
We're in trouble whenever the width of the HWIs is different
from the width of the constant they represent.

> @@ -103,11 +123,16 @@ 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))
> +	  if (GET_MODE_BITSIZE (mode) > 2*HOST_BITS_PER_WIDE_INT)
> +	    /* Sorry, we have no way to represent overflows this
> +	       wide.  To fix, add constant support wider than
> +	       CONST_DOUBLE.  */
> +	    gcc_assert (0);

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)

(note spacing).

> @@ -121,8 +146,8 @@ plus_constant (rtx x, HOST_WIDE_INT c)
>  	{
>  	  tem
>  	    = force_const_mem (GET_MODE (x),
> -			       plus_constant (get_pool_constant (XEXP (x, 0)),
> -					      c));
> +			       plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)),
> +						   c));
>  	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
>  	    return tem;
>  	}

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);

> diff --git a/gcc/expmed.c b/gcc/expmed.c
> index 3507dad..2361b7e 100644
> --- a/gcc/expmed.c
> +++ b/gcc/expmed.c
> @@ -3122,9 +3122,11 @@ expand_mult (enum machine_mode mode, rtx op0, rtx op1, rtx target,
>  	    {
>  	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
>  			  + HOST_BITS_PER_WIDE_INT;
> -	      return expand_shift (LSHIFT_EXPR, mode, op0,
> -				   build_int_cst (NULL_TREE, shift),
> -				   target, unsignedp);
> +	      if (shift < 2*HOST_BITS_PER_WIDE_INT-1
> +		  || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT)

Missing spaces around binary operators.

> @@ -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.  I think it
should be something like:

      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 if (GET_MODE_BITSIZE (op_mode) <= HOST_BITS_PER_WIDE_INT)
	hv = 0, lv &= GET_MODE_MASK (op_mode);

> @@ -2214,7 +2210,9 @@ simplify_binary_operation_1 (enum rtx_code code, enum machine_mode mode,
>  	      || 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));
>  

Missing spaces around binary operators.

OK with those changes as far as the RTL optimisations go.  I'm happy
with the rest too, but despite all this fuss, I can't approve the
immed_double_const change itself.  Sounds like Richard G would be
willing though.

Richard