This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: remove wrong code in immed_double_const
On Mar 21, 2012, at 6:17 AM, Richard Sandiford wrote:
> Sounds good.
Cool, a path forward.
> For this I think we should make plus_constant a wrapper:
Ah, yes, much better, thanks. I'd expanded the comments on plus_constant_mode so that one might have a better idea why the code is that way and put in a TODO, so that people have an idea of what direction the code wants to move.
> I don't think it's a good idea to punt to a PLUS though.
Fixed, thanks for the code.
> Nicely, add_double returns true on overflow, so I think
> we should replace the punt with:
Ah, yes, nice, fixed.
> For this I think we should change the recursive CONSTANT_P call
> to use plus_constant_mode
Done.
> and we can remove the special CONST_INT case.
Ok, ah, yes, because the recursive call will already combine them, done.
>> 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.
Ah, I saw it, but missed it anyway, thanks for the pointer, fixed.
> 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. */
Done.
>> @@ -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.
Ah, yes, I agree an assert would be better, as really, no one should ask for an unsigned conversion to floating point on a value that is negative, more likely it is just a spot we missed adding an assert to earlier and probably wants a larger constant that can represent a large unsigned number. Fixed.
> 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.
> I think the expand_mult CONST_DOUBLE code needs changing
Agreed. Fixed. I preserve the code for smaller modes, and for small enough shift amounts. 1<<67 for example, or any mode, is still handled. For large enough things, we just don't return the shift.
> Same for:
Fixed, in same way as previous case.
> 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
? If so, we already know that the width is <= HOST_BITS_PER_WIDE_INT * 2. :-)
Thanks for spotting the bits I missed.
Current patch:
diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texiindex 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
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)
{
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. */
+/* 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;
@@ -90,12 +96,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:
@@ -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);
return immed_double_const (lv, hv, VOIDmode);
}
@@ -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;
}
@@ -141,31 +166,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))
@@ -175,7 +186,7 @@ plus_constant (rtx x, HOST_WIDE_INT c)
rtx copy = copy_rtx (x);
rtx *const_loc = find_constant_term_loc (©);
- *const_loc = plus_constant (*const_loc, c);
+ *const_loc = plus_constant_mode (mode, *const_loc, c);
x = copy;
c = 0;
}
@@ -195,6 +206,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.
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)
+ return expand_shift (LSHIFT_EXPR, mode, op0,
+ build_int_cst (NULL_TREE, shift),
+ target, unsignedp);
}
}
diff --git a/gcc/rtl.h b/gcc/rtl.h
index 66f2755..9d1a28e 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -1586,6 +1586,7 @@ extern int ceil_log2 (unsigned HOST_WIDE_INT);
/* 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);
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index ce4eab4..ff838a8 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, const_rtx x)
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)
@@ -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);
@@ -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));
@@ -4987,6 +4985,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 +4998,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;
}
else
{
+ unsigned char extend = 0;
long tmp[max_bitsize / 32];
int bitsize = GET_MODE_BITSIZE (GET_MODE (el));
@@ -5030,10 +5031,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;
}
break;