Mike Stump <mikest...@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
