| Message ID | DF6339D4-BFF6-45FA-A5E0-8141F0BD4199@comcast.net |
|---|---|
| State | New |
| Headers | show |
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
Richard Sandiford <rdsandiford@googlemail.com> writes: > 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) Er, I did of course mean: gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT) :-) Richard
Hi, On Wed, 21 Mar 2012, Mike Stump wrote: > --- 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. I see that you didn't remove the assert as part of this patch. I'd like to see what you like to do to this routine once the rest goes in. In particular I don't think just removing the assert will be enough, at the very least the block comment should be saying something about what the routine exactly does (or doesn't do) for modes where the two HWI arguments can't specify all bits. My point is, for large modes i0 and i1 will only specify the low 2*HWIbits bits. Something needs to document what the upper bits will be (be they implicit or explicit), otherwise people reading the comment will always wonder what exactly is supposed to happen. I'm not 100% sure what it should say, though. Probably the interpretation of the upper bits depends on the users of the so generated CONST_DOUBLEs. Ciao, Michael.
On Mar 22, 2012, at 7:12 AM, Michael Matz wrote: > I see that you didn't remove the assert as part of this patch. I'll include that in my next patch. > I'd like to see what you like to do to this routine once the rest goes in. In > particular I don't think just removing the assert will be enough, at the > very least the block comment should be saying something about what the > routine exactly does (or doesn't do) for modes where the two HWI arguments > can't specify all bits. I think the best approach is to refine the spec: /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair of ints: I0 is the low-order word and I1 is the high-order word. The value is a signed value, with the high bit of i1 being the sign bit. Do not use this routine for non-integer modes; convert to REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ I think this then exactly matches CONST_DOUBLE semantics.
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;