Mike Stump <[email protected]> writes:
> diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
> index de45a22..0c6dc45 100644
> --- a/gcc/doc/rtl.texi
> +++ b/gcc/doc/rtl.texi
> @@ -1530,7 +1530,9 @@ Represents either a floating-point constant of mode
> @var{m} or an
> 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
Sounds good.
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 78ddfc3..c0b24e4 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -531,10 +531,9 @@ immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1,
> enum machine_mode mode)
>
> 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)
> {
This too.
> diff --git a/gcc/explow.c b/gcc/explow.c
> index 2fae1a1..6284d61 100644
> --- a/gcc/explow.c
> +++ b/gcc/explow.c
> @@ -96,6 +96,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)
For this I think we should make plus_constant a wrapper:
/* 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);
}
/* Return an rtx for the sum of X and the integer C, given that X
has mode MODE. */
rtx
plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
{
...innards of current plus_constant, without the "mode = "...
}
Reason being...
> switch (code)
> {
> case CONST_INT:
> + if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
> + /* Punt for now. */
> + goto overflow;
> return GEN_INT (INTVAL (x) + c);
>
> case CONST_DOUBLE:
...this won't work as things stand, since CONST_INT always has VOIDmode.
(I'm on a slow mission to fix that.)
I agree that this is a pre-existing bug. Callers that want to be
CONST_INT-safe should use the new plus_constant_mode instead of
plus_constant. Once they do, we should assert here that mode isn't
VOIDmode. But since it's an existing bug that also affects 2-HWI
constants, I agree that replacing calls to plus_constant with calls
to plus_constant_mode is a separate fix.
I don't think it's a good idea to punt to a PLUS though.
(plus (const_int X) (const_int Y)) isn't canonical rtl,
and could cause other problems.
Suggest instead we reuse the CONST_DOUBLE code for CONST_INT,
with l1 set to INTVAL and h1 set to the sign extension.
> @@ -103,10 +106,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 ? ~(HOST_WIDE_INT)0 : 0;
> unsigned HOST_WIDE_INT lv;
> HOST_WIDE_INT hv;
>
> + if (GET_MODE_BITSIZE (mode) > 2*HOST_WIDE_INT)
> + /* Punt for now. */
> + goto overflow;
> +
> add_double (l1, h1, l2, h2, &lv, &hv);
Nicely, add_double returns true on overflow, so I think
we should replace the punt with:
if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT);
(Seems better to explicitly specify the sign, even though
add_double would be equivalent.)
> @@ -141,6 +148,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)
> break;
>
> case PLUS:
> + if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
> + /* Punt for now. */
> + goto overflow;
> /* 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
For this I think we should change the recursive CONSTANT_P call
to use plus_constant_mode (with the mode of the PLUS) instead of
plus_constant. It will then be correct for CONST_INT, and we can
remove the special CONST_INT case.
> diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
> index ce4eab4..37e46b1 100644
> --- a/gcc/simplify-rtx.c
> +++ b/gcc/simplify-rtx.c
> @@ -101,6 +101,7 @@ mode_signbit_p (enum machine_mode mode, const_rtx x)
>
> if (width < HOST_BITS_PER_WIDE_INT)
> val &= ((unsigned HOST_WIDE_INT) 1 << width) - 1;
> + /* FIXME: audit for TImode and wider correctness. */
> return val == ((unsigned HOST_WIDE_INT) 1 << (width - 1));
We've already returned false in that case though. I'm happy for this
function to be left as-is, but we could instead add a comment like:
/* FIXME: We don't yet have a representation for wider modes. */
above the:
return false;
> @@ -1227,7 +1228,7 @@ simplify_const_unary_operation (enum rtx_code code,
> enum machine_mode mode,
> return 0;
> }
> else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
> - ;
> + return 0;
> else
> hv = 0, lv &= GET_MODE_MASK (op_mode);
Should keep the current behaviour for == HOST_BITS_PER_WIDE_INT * 2.
I'd slightly prefer an assert rather than a "return false", but I won't
argue if another maintainer agrees with the return.
> @@ -4987,6 +4988,7 @@ simplify_immed_subreg (enum machine_mode outermode, rtx
> op,
> 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);
> @@ -4999,13 +5001,15 @@ simplify_immed_subreg (enum machine_mode outermode,
> rtx op,
> = 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;
> }
Looks good.
> else
> {
> + unsigned char extend = 0;
> long tmp[max_bitsize / 32];
> int bitsize = GET_MODE_BITSIZE (GET_MODE (el));
>
> @@ -5030,10 +5034,10 @@ simplify_immed_subreg (enum machine_mode outermode,
> rtx op,
> *vp++ = tmp[ibase / 32] >> i % 32;
> }
>
> - /* 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;
> }
This is changing the real case, where sign extension doesn't make
much sense.
I think the expand_mult CONST_DOUBLE code needs changing too:
else if (CONST_DOUBLE_LOW (op1) == 0
&& EXACT_POWER_OF_2_OR_ZERO_P (CONST_DOUBLE_HIGH (op1)))
{
int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
+ HOST_BITS_PER_WIDE_INT;
return expand_shift (LSHIFT_EXPR, mode, op0,
shift, target, unsignedp);
}
This isn't correct if the mode is wider than 2 HWIs and
CONST_DOUBLE_HIGH has the high bit set.
Same for:
/* Likewise for multipliers wider than a word. */
if (GET_CODE (trueop1) == CONST_DOUBLE
&& (GET_MODE (trueop1) == VOIDmode
|| 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)
return simplify_gen_binary (ASHIFT, mode, op0,
GEN_INT (val + HOST_BITS_PER_WIDE_INT));
in the MULT case of simplify_binary_operation_1.
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;
and similarly for other cases.
Richard
