Re: remove wrong code in immed_double_const
On Mar 26, 2012, at 4:57 PM, Mike Stump wrote: > On Mar 26, 2012, at 1:03 PM, Richard Sandiford wrote: >> I think: >> >> ...copies of the top bit. Note however that values are neither inherently >> signed nor inherently unsigned; where necessary, signedness is determined >> by the rtl operation instead. > > Sounds good to me, changed. Oh, review caught one last problem: > +, however values are neither signed nor unsigned. > +All operations defined on such constants define the signededness. This was edit cruft from the last rewording for the doc, the cruft has been removed. * doc/rtl.texi (const_double): Document as sign-extending. * expmed.c (expand_mult): Ensure we don't use shift incorrectly. * emit-rtl.c (immed_double_int_const): Refine to state the value is signed. * simplify-rtx.c (mode_signbit_p): Add a fixme for wider than CONST_DOUBLE integers. (simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no negative values are converted. Fix conversions bigger than HOST_BITS_PER_WIDE_INT. (simplify_binary_operation_1): Ensure we don't use shift incorrectly. (simplify_immed_subreg): Sign-extend CONST_DOUBLEs. * explow.c (plus_constant_mode): Add. (plus_constant): Implement with plus_constant_mode. * rtl.h (plus_constant_mode): Add. Index: doc/rtl.texi === --- doc/rtl.texi(revision 186111) +++ doc/rtl.texi(working copy) @@ -1479,8 +1479,13 @@ This type of expression represents the i is customarily accessed with the macro @code{INTVAL} as in @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}. -Constants generated for modes with fewer bits than @code{HOST_WIDE_INT} -must be sign extended to full width (e.g., with @code{gen_int_mode}). +Constants generated for modes with fewer bits than in +@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with +@code{gen_int_mode}). For constants for modes with more bits than in +@code{HOST_WIDE_INT} the implied high order bits of that constant are +copies of the top bit. Note however that values are neither +inherently signed nor inherently unsigned; where necessary, signedness +is determined by the rtl operation instead. @findex const0_rtx @findex const1_rtx @@ -1510,7 +1515,13 @@ Represents either a floating-point const 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 +constants for modes with more bits than twice the number in +@code{HOST_WIDE_INT} the implied high order bits of that constant are +copies of the top bit of @code{CONST_DOUBLE_HIGH}. Note however that +integral values are neither inherently signed nor inherently unsigned; +where necessary, signedness is determined by the rtl operation +instead. @findex CONST_DOUBLE_LOW If @var{m} is @code{VOIDmode}, the bits of the value are stored in Index: expmed.c === --- expmed.c(revision 186111) +++ expmed.c(working copy) @@ -3139,8 +3139,10 @@ expand_mult (enum machine_mode mode, rtx { int shift = floor_log2 (CONST_DOUBLE_HIGH (op1)) + HOST_BITS_PER_WIDE_INT; - return expand_shift (LSHIFT_EXPR, mode, op0, - 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, +shift, target, unsignedp); } } Index: emit-rtl.c === --- emit-rtl.c (revision 186111) +++ emit-rtl.c (working copy) @@ -517,8 +517,11 @@ immed_double_int_const (double_int i, en /* 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. - Do not use this routine for non-integer modes; convert to - REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ + For values that are larger than 2*HOST_BITS_PER_WIDE_INT, the + implied upper bits are copies of the high bit of i1. The value + itself is neither signed nor unsigned. Do not use this routine for + non-integer modes; convert to REAL_VALUE_TYPE and use + CONST_DOUBLE_FROM_REAL_VALUE. */ rtx immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode) @@ -531,10 +534,9 @@ immed_double_const (HOST_WIDE_INT i0, HO 1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
Re: remove wrong code in immed_double_const
On Mar 26, 2012, at 1:03 PM, Richard Sandiford wrote: > I think: > > ...copies of the top bit. Note however that values are neither inherently > signed nor inherently unsigned; where necessary, signedness is determined > by the rtl operation instead. Sounds good to me, changed. > Same idea here. Fixed. >> +/* 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 > > Sorry, just noticed, should be "...when X can be..." Fixed. Richard has ok all his bits, now down to you for final ok. Ok? * doc/rtl.texi (const_double): Document as sign-extending. * expmed.c (expand_mult): Ensure we don't use shift incorrectly. * emit-rtl.c (immed_double_int_const): Refine to state the value is signed. * simplify-rtx.c (mode_signbit_p): Add a fixme for wider than CONST_DOUBLE integers. (simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no negative values are converted. Fix conversions bigger than HOST_BITS_PER_WIDE_INT. (simplify_binary_operation_1): Ensure we don't use shift incorrectly. (simplify_immed_subreg): Sign-extend CONST_DOUBLEs. * explow.c (plus_constant_mode): Add. (plus_constant): Implement with plus_constant_mode. * rtl.h (plus_constant_mode): Add. Index: doc/rtl.texi === --- doc/rtl.texi(revision 185706) +++ doc/rtl.texi(working copy) @@ -1479,8 +1479,19 @@ This type of expression represents the i is customarily accessed with the macro @code{INTVAL} as in @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}. -Constants generated for modes with fewer bits than @code{HOST_WIDE_INT} -must be sign extended to full width (e.g., with @code{gen_int_mode}). +Constants generated for modes with fewer bits than in +@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with +@code{gen_int_mode}). For constants for modes with more bits than in +@code{HOST_WIDE_INT} the implied high order bits of that constant are +copies of the top bit. Note however that values are neither +inherently signed nor inherently unsigned; where necessary, signedness +is determined by the rtl operation instead. + + +, however values are neither signed nor unsigned. +All operations defined on such constants define the signededness. + + @findex const0_rtx @findex const1_rtx @@ -1510,7 +1521,13 @@ Represents either a floating-point const 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 +constants for modes with more bits than twice the number in +@code{HOST_WIDE_INT} the implied high order bits of that constant are +copies of the top bit of @code{CONST_DOUBLE_HIGH}. Note however that +integral values are neither inherently signed nor inherently unsigned; +where necessary, signedness is determined by the rtl operation +instead. @findex CONST_DOUBLE_LOW If @var{m} is @code{VOIDmode}, the bits of the value are stored in Index: expmed.c === --- expmed.c(revision 185706) +++ expmed.c(working copy) @@ -3135,8 +3135,10 @@ expand_mult (enum machine_mode mode, rtx { int shift = floor_log2 (CONST_DOUBLE_HIGH (op1)) + HOST_BITS_PER_WIDE_INT; - return expand_shift (LSHIFT_EXPR, mode, op0, - 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, +shift, target, unsignedp); } } Index: emit-rtl.c === --- emit-rtl.c (revision 185706) +++ emit-rtl.c (working copy) @@ -517,8 +517,11 @@ immed_double_int_const (double_int i, en /* 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. - Do not use this routine for non-integer modes; convert to - REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ + For values that are larger than 2*HOST_BITS_PER_WIDE_INT, the + implied upper bits are copies of the high bit of i1. The value + itself is neither signed nor unsigned. Do not use this routine for + non-integer modes; convert to REAL_VALUE_TYPE and use +
Re: remove wrong code in immed_double_const
Mike Stump writes: > On Mar 23, 2012, at 3:01 AM, Richard Sandiford wrote: >> ...it doesn't mean that we interpret the value as a negative _rtx_. >> As with all rtx calculations, things like signedness and saturation are >> decided by the operation rather than the "type" ("type" == rtx mode). > > Ah... [ light goes on ] Let me adjust the documentation to be exceptionally > clear in this case. Check out the new wording on const_int, const_double and > on immed_double_const. I fixed simplify_const_unary_operation to match your > suggestion. > >> Sorry for the rather rambling explanation :-) I still think the >> version I suggested for this hunk is right though. > > I agree. I now see what I had wrong. Thanks for your patience and > explanations. If you like the wording I used in the doc and on > immed_double_const, I think we have now resolved all issues. The previous > version was bootstraped and regression tested on darwin, fortran, c, c++, > objective-c++... I'll do one more run with the update for > simplify_const_unary_operation, as that can trip when before it would merely > return 0. > > Ok? > > > Index: doc/rtl.texi > === > --- doc/rtl.texi (revision 185706) > +++ doc/rtl.texi (working copy) > @@ -1479,8 +1479,13 @@ This type of expression represents the i > is customarily accessed with the macro @code{INTVAL} as in > @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, > 0)}. > > -Constants generated for modes with fewer bits than @code{HOST_WIDE_INT} > -must be sign extended to full width (e.g., with @code{gen_int_mode}). > +Constants generated for modes with fewer bits than in > +@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with > +@code{gen_int_mode}). For constants for modes with more bits than in > +@code{HOST_WIDE_INT} the implied high order bits of that constant are > +copies of the top bit, however values are neither signed nor unsigned. > +All operations defined on such constants define the signededness. I'm not someone who should be wordsmithing, but I think: ...copies of the top bit. Note however that values are neither inherently signed nor inherently unsigned; where necessary, signedness is determined by the rtl operation instead. > @@ -1510,7 +1515,12 @@ Represents either a floating-point const > 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 > +constants for modes with more bits than twice the number in > +@code{HOST_WIDE_INT} the implied high order bits of that constant are > +copies of the top bit of @code{CONST_DOUBLE_HIGH}, however integral > +values are neither signed nor unsigned. All operations defined on > +such constants define the signededness. Same idea here. > +/* 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 Sorry, just noticed, should be "...when X can be..." Looks good to me otherwise, thanks. Richi? Richard
Re: remove wrong code in immed_double_const
On Mar 23, 2012, at 3:01 AM, Richard Sandiford wrote: > ...it doesn't mean that we interpret the value as a negative _rtx_. > As with all rtx calculations, things like signedness and saturation are > decided by the operation rather than the "type" ("type" == rtx mode). Ah... [ light goes on ] Let me adjust the documentation to be exceptionally clear in this case. Check out the new wording on const_int, const_double and on immed_double_const. I fixed simplify_const_unary_operation to match your suggestion. > Sorry for the rather rambling explanation :-) I still think the > version I suggested for this hunk is right though. I agree. I now see what I had wrong. Thanks for your patience and explanations. If you like the wording I used in the doc and on immed_double_const, I think we have now resolved all issues. The previous version was bootstraped and regression tested on darwin, fortran, c, c++, objective-c++... I'll do one more run with the update for simplify_const_unary_operation, as that can trip when before it would merely return 0. Ok? Index: doc/rtl.texi === --- doc/rtl.texi(revision 185706) +++ doc/rtl.texi(working copy) @@ -1479,8 +1479,13 @@ This type of expression represents the i is customarily accessed with the macro @code{INTVAL} as in @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}. -Constants generated for modes with fewer bits than @code{HOST_WIDE_INT} -must be sign extended to full width (e.g., with @code{gen_int_mode}). +Constants generated for modes with fewer bits than in +@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with +@code{gen_int_mode}). For constants for modes with more bits than in +@code{HOST_WIDE_INT} the implied high order bits of that constant are +copies of the top bit, however values are neither signed nor unsigned. +All operations defined on such constants define the signededness. + @findex const0_rtx @findex const1_rtx @@ -1510,7 +1515,12 @@ Represents either a floating-point const 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 +constants for modes with more bits than twice the number in +@code{HOST_WIDE_INT} the implied high order bits of that constant are +copies of the top bit of @code{CONST_DOUBLE_HIGH}, however integral +values are neither signed nor unsigned. All operations defined on +such constants define the signededness. @findex CONST_DOUBLE_LOW If @var{m} is @code{VOIDmode}, the bits of the value are stored in Index: expmed.c === --- expmed.c(revision 185706) +++ expmed.c(working copy) @@ -3135,8 +3135,10 @@ expand_mult (enum machine_mode mode, rtx { int shift = floor_log2 (CONST_DOUBLE_HIGH (op1)) + HOST_BITS_PER_WIDE_INT; - return expand_shift (LSHIFT_EXPR, mode, op0, - 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, +shift, target, unsignedp); } } Index: emit-rtl.c === --- emit-rtl.c (revision 185706) +++ emit-rtl.c (working copy) @@ -517,8 +517,11 @@ immed_double_int_const (double_int i, en /* 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. - Do not use this routine for non-integer modes; convert to - REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ + For values that are larger than 2*HOST_BITS_PER_WIDE_INT, the + implied upper bits are copies of the high bit of i1. The value + itself is neither signed nor unsigned. Do not use this routine for + non-integer modes; convert to REAL_VALUE_TYPE and use + CONST_DOUBLE_FROM_REAL_VALUE. */ rtx immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode) @@ -531,10 +534,9 @@ immed_double_const (HOST_WIDE_INT i0, HO 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 o
Re: remove wrong code in immed_double_const
Mike Stump writes: >> 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. (To be clear, I was using uint128_t as an example of a 2-HWI type, assuming we're using 64-bit HWIs -- which I hope we are for targets where this assert matters.) > Well, according to the spec, one cannot use CONST_DOUBLE to represent > a uint128 value with the high bit set. We can! And do now, even without your patch. Because... > The C frontend type plays this game, but they can, because they track > the type with the constant the the values of the constant are > interpreted exclusively in the context of the type. Since we don't > have the unsigned bit, we can't, so, either, they are speced to be > values on their own, or values dependent upon some external notion. > By changing the spec to say sign extending, we mean if the high bit is > set, the value is negative. ...it doesn't mean that we interpret the value as a negative _rtx_. As with all rtx calculations, things like signedness and saturation are decided by the operation rather than the "type" ("type" == rtx mode). For things like addition where signed vs. unsigned interpretation doesn't matter, we have a single rtx op like PLUS. For things like multiplication where it does matter, we have separate signed and unsigned variants. There is nothing to distinguish a uint128_t _register_ (i.e. TImode REG) that has the upper bit set from an int128_t register that happens to be negative. Instead the interpretation is decided by the operation. And the same principle applies to constants. There isn't, and doesn't need to be, a separate CONST_DOUBLE representation for: - an unsigned 2-HWI value that has the upper bit set and - a signed 2-HWI value that is negative The sign-extending thing is simply there to specify what happens when an N>2 HWI value is represented as a 2-HWI rtx. I.e. it's simply there to say what the implicit N-2 HWIs are. (That's why the definition only matters now that we're allowing N>2 by removing the assert.) In this context we're interpreting the value as unsigned because we have an UNSIGNED_FLOAT operation. So if the mode of the operand is exactly 2 HWIs in size, a negative high HWI simply indicates an unsigned value that has the high bit set. The same principle already applies to CONST_INT. We have long defined CONST_INT to be a sign-extending representation, in the sense that it is allowed to represent 2-HWI modes in which the upper HWI happens to be a sign extension of the lower HWI. That doesn't mean the 2-HWI constant itself is negative: it can just as easily be a high unsigned value. Whether it is signed, unsigned or neutral depends on the context of the rtx operation. All we're doing here is extending that principle to CONST_DOUBLE and modes wider than 2 HWIs. Sorry for the rather rambling explanation :-) I still think the version I suggested for this hunk is right though. Richard
Re: remove wrong code in immed_double_const
On Mar 22, 2012, at 3:16 AM, Richard Sandiford wrote: > Sorry, meant we should leave the svn version as it is. Ah, I see now, I agree, I removed that bit (extend in the floating point case) of my change from the patch. > I think this should be s/is smaller than/is different from/. Sounds good, fixed. > 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) As you noted, you do mean: 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) Fixed. > 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); Fixed. >> + if (shift < 2*HOST_BITS_PER_WIDE_INT-1 >> + || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT) > > Missing spaces around binary operators. Fixed all instances of 2*HOST and INT-1, there were many, not just this one. Some pre-date my patch. >> @@ -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. Well, according to the spec, one cannot use CONST_DOUBLE to represent a uint128 value with the high bit set. The C frontend type plays this game, but they can, because they track the type with the constant the the values of the constant are interpreted exclusively in the context of the type. Since we don't have the unsigned bit, we can't, so, either, they are speced to be values on their own, or values dependent upon some external notion. By changing the spec to say sign extending, we mean if the high bit is set, the value is negative. If this is the wrong value for a uint value, then that representation cannot be used. The only solution is to use a different representation (CONST_QUAD, CONST_UDOUBLE or something else). I'm thought about that routine some more, and it is just totally broken. I've changed the code to: /* We should never get a negative number. */ gcc_assert (hv >= 0); if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT) hv = 0, lv &= GET_MODE_MASK (op_mode); negative numbers are bad, period, this badness, doesn't depend upon anything other than the value being negative. The use of hv = 0, can only be done with PRECISIONs less than or equal to HOST_BITS_PER_WIDE_INT. Wider than this, and some of hv would be wiped out, which would be wrong. Thoughts? > OK with those changes as far as the RTL optimisations go. Not quite. Need to resolve the point just above. But, yea, we are getting very close now. I'm switched over to a patch for trunk (from 4.6.0) and added the ChangeLog. The switch to GET_MODE_PRECISION above was one change, for example. The rest fit in nicely, the arg to expand_shift was updated slightly to match trunk. > I can't approve the immed_double_const change itself. Sounds like Richard G > would be > willing though. I've bundled in the patch that started this all into this version. I'll look forward to his review. * doc/rtl.texi (const_double): Document as sign-extending. * expmed.c (expand_mult): Ensure we don't use shift incorrectly. * emit-rtl.c (immed_double_int_const): Refine to state the value is signed. * simplify-rtx.c (mode_signbit_p): Add a fixme for wider than CONST_DOUBLE integers. (simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no negative values are converted. Fix conversions bigger than HOST_BITS_PER_WIDE_INT.
Re: remove wrong code in immed_double_const
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.
Re: remove wrong code in immed_double_const
On Mar 22, 2012, at 6:15 AM, Michael Matz wrote: > That certainly is strictly better than any of the other possibilities, I > just didn't get the impression from your second mail to this thread that > you were even considering doing that. Good I was wrong. All I wanted, was to remove the assert... The review point was, no, not unless you fix the issues so we don't get wrong code-gen and could you make it sign extending as well? So, sure, sounds reasonable to me. I was going to do the work in the end, just didn't plan on doing it today. Today, tomorrow, not worth quibbling over the exact date the work is done. But, my final point is, the assert is wrong, and it has to go, and (almost) gone it is. :-) I'm happy. > I would call it too strict, not wrong. Do you have users? Have you ever told them the compiler isn't wrong when it ICEs for perfectly valid code? I've never done that before, and never plan to, no one has convinced me it is the right approach. If you want me to not use the term wrong, you'd need to furnish a web site that somehow proves your point. Wrong is what I use when that the compiler does is wrong. It is that simple. Failing to compile valid code, is wrong. > Because there are (or were after > your fixes get it) values where there was a problem. Of course that's > again just splitting hair about terminology :) Yeah, I'm not into hair splitting on terminology. I'm more into actual functionality of the compiler to the end user.
Re: remove wrong code in immed_double_const
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.
Re: remove wrong code in immed_double_const
Hi, On Wed, 21 Mar 2012, Mike Stump wrote: > > If the high bit of i1 is set then currently you will get a negative > > number produced no matter the absolute value of it. > > Ok, in the new patch, I'm pushing to change the spec so that the value > is sign extended and fixing all the code that doesn't conform to that > spec. That certainly is strictly better than any of the other possibilities, I just didn't get the impression from your second mail to this thread that you were even considering doing that. Good I was wrong. If I notice anything I'll answer directly to the patch. > > This positive/negative inconsistency doesn't make sense to allow, and > > the assert ensures that it isn't allowed. > > Don't need the assert when there is no inconsistency, I believe that > resolving any inconsistencies should remove the need for the assert. Correct. > :-) Only the point I wanted to make; that 0 is safe. As such, it > proves that the spec, as you might call it, is wrong. I would call it too strict, not wrong. Because there are (or were after your fixes get it) values where there was a problem. Of course that's again just splitting hair about terminology :) Ciao, Michael.
Re: remove wrong code in immed_double_const
Richard Sandiford 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
Re: remove wrong code in immed_double_const
Mike Stump 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) > -
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 any
Re: remove wrong code in immed_double_const
On Mar 21, 2012, at 6:47 AM, Michael Matz wrote: > Actually, I wouldn't. Ok, thanks for explaining. In light of that, I'd just say, I want to change the spec, the details don't change any for me, only the terminology I might use. The problem is the spec is causing aborts on valid code, and hence, either, the code must be duplicated and fixed, or the code has to be fixed. I don't see any value in duplicating the code, so, I am left with fixing the spec so that valid programs produce valid code. > If the high bit of i1 is set then currently you will get > a negative number produced no matter the absolute value of it. Ok, in the new patch, I'm pushing to change the spec so that the value is sign extended and fixing all the code that doesn't conform to that spec. Richard seems to be agreeable with the basic idea, though, we are now sorting out all the little details to make that happen. If there is any down-side or details we missed or got wrong, please chime in. > For large negative numbers you'll get a CONST_DOUBLE, > that when interpreted in the large requested mode (which is the only thing > that makes sense) is positive. In the new patch, we use sign extension, and when the high bit is set, the value is interpreted as a negative number is a larger mode. I'll test signed and unsigned constants coming in from above to ensure the right thing happens. Above the signededness is tracked via the type. In the rtl constant, it isn't, so that code will need an assert to prevent large unsigned values from taking this code path. > Hence all values where i1 is between (HWI)1 << (hwibits-1)) and > ((HWI)~0)-1 are the values you're searching for, that show the problem. Presently, I am fixing _all_ problems shown with those values. If you know of any that we don't address, love to hear about them. > This positive/negative inconsistency doesn't make sense to allow, and the > assert ensures that it isn't allowed. Don't need the assert when there is no inconsistency, I believe that resolving any inconsistencies should remove the need for the assert. > This would have the seemingly strange effect of disallowing too large > modes only for large values, but that's simply a side-effect of > CONST_DOUBLE and the whole associated machinery not being able to > consistently deal with constants wider than 2*HWI_BITS. I'll move that assert up to the code that has the type information for the constant. >> if I is 42, we abort. To back the position that spec must not be >> changed, you need to explain at least one thing for which the wrong >> thing will happen if the spec did change. If you want to go down that >> path, you will need to furnish one example where badness happens with 0, >> not 2, not 3, but 0. > > Huh. Removing the assert wouldn't only allow 0, but also other values. > I don't understand your argumentation: "because for 0 nothing bad happens, > that proves that nothing bad happens for any other values which we would > also allow, hence I can remove the assert"? Right, it merely proves that the assert is wrong and needs fixing. Once you accept that, then we progress on exactly what it should be. This is now all mostly moot, given that I'm fine with changing the spec as Richard suggested to be a sign-extended constant. Once we have that nice are concrete definition, the any code conflicts with that, is buggy, and we just fix. Seems like a nice way forward to me. > It of course doesn't prove anything at all. :-) Only the point I wanted to make; that 0 is safe. As such, it proves that the spec, as you might call it, is wrong. Once that spec is proven wrong, trivially, fixing it, isn't unreasonable.
Re: remove wrong code in immed_double_const
Hi, On Tue, 20 Mar 2012, Mike Stump wrote: > > Actually you did. I've tried yesterday to come up with a text that > > would do the same (because I agree with you that deleting the assert > > changes the spec of the function, > > The spec of the function is the text above the definition of the > function, coupled with the information in the .texi file, would you > agree? Actually, I wouldn't. The real spec includes many pieces of information, the comments (that can be incomplete or become out of date), the .texi docu (which can be even more incomplete and out of date), the code (which can conflict with the comments and still be the correct variant) and the current usage (which can conflict with everything of the above). asserts are IMO even a nice way of documenting parts of the spec. > If so, could you please quote the text of the spec which would > be violated by removing the assert? Could you please give a specific > value with which we could talk about that shows a violation of the spec. Richard did so. If the high bit of i1 is set then currently you will get a negative number produced no matter the absolute value of it. That's IMO part of the spec, high bit set --> negative number. negative as defined by the various routines dealing with CONST_INT or CONST_DOUBLE interpreted in the modes allowed for creating them. If you were to allow modes of larger size than what could be completely specified with the two HWI arguments i0 and i1, then it suddenly depends on the absolute value if you get a negative or positive number. For small negative numbers (those for which i1 is ~0 and i0 < 0) you'll still get a negative CONST_INT. For large negative numbers you'll get a CONST_DOUBLE, that when interpreted in the large requested mode (which is the only thing that makes sense) is positive. It doesn't matter that it's still negative when interpreted in a smaller mode. Hence all values where i1 is between (HWI)1 << (hwibits-1)) and ((HWI)~0)-1 are the values you're searching for, that show the problem. As you correctly note the routine will of course generate the exact same CONST_DOUBLE object no matter the mode given, but they have to be interpreted together with the given mode. This positive/negative inconsistency doesn't make sense to allow, and the assert ensures that it isn't allowed. Now, this inconsistency can also be avoided via different means. By extending the block comment in front of the function for instance, but then the assert would make even more sense. So Richards proposal to move the assert is better: The problem occurs only with large positive or negative values (i1 not 0 or ~0), so the mode-size check can be moved after the GEN_INT call. This would have the seemingly strange effect of disallowing too large modes only for large values, but that's simply a side-effect of CONST_DOUBLE and the whole associated machinery not being able to consistently deal with constants wider than 2*HWI_BITS. > My position is simple, the spec is what is above the definition and the > .texi files, and the stuff inside the definition are interesting > implementation details of that spec, which _never_ modify the spec. As an abstract goal that's good. But reality is that this isn't the case. GCC is quite excellently commented, but it doesn't fit the ideal. Using the fact that it isn't ideal to claim that the spec doesn't say anything about large modes (when the assert clearly disallows them) is absurd. > My position is that 0 is a value which the spec defines, and for which > we assert. Please quote the line from the spec that defines what we do > in that case. I've never seen anyone quote such a line. To support > your position, I will insist on a direct quote from the spec. This line disallows the value 0 with large modes: gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); I insist on it being part of the spec. Moving the assert changes the spec to allow 0 (and generally small positive and negative numbers) to also be generated for larger modes. If you so want to change the spec nobody would be opposed. > if I is 42, we abort. To back the position that spec must not be > changed, you need to explain at least one thing for which the wrong > thing will happen if the spec did change. If you want to go down that > path, you will need to furnish one example where badness happens with 0, > not 2, not 3, but 0. Huh. Removing the assert wouldn't only allow 0, but also other values. I don't understand your argumentation: "because for 0 nothing bad happens, that proves that nothing bad happens for any other values which we would also allow, hence I can remove the assert"? It of course doesn't prove anything at all. In any case, above I have given the values that will be problematic (and they don't include 0), and a way of changing the spec to disallow only them, instead of all values. Actually Richard S.
Re: remove wrong code in immed_double_const
Mike Stump 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 @@ m
Re: remove wrong code in immed_double_const
On Mar 20, 2012, at 5:26 AM, Richard Sandiford wrote: > So what I was trying to say was that if we remove the assert > altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs, > we need to define what the "implicit" high-order HWIs of a > CONST_DOUBLE are, just like we already do for CONST_INT. Now, since you expressed a preference for sign extending, and a worry that there might be new bugs exposed in the handling of CONST_DOUBLEs in the face of my change, I went through all the code again and tried my best to fix every other bug in the compiler at all related to this area that I could find ; that patch is below. In this one, I updated the spec for CONST_DOUBLE to be sign extending. Curious, plus_constant is just terribly broken in this are, now fixed. mode_signbit_p is speced in English, so, I didn't want to misread or misunderstand it and swizzle it, so I left it alone for now. Someone will have to describe what it does and I can try my hand at fixing it, if broken, I suspect it is. As for simplify_const_unary_operation, I don't know what they were thinking, return 0 seems safer to me. If there is any other code that I missed that people know about, I'd be happy to fix it, just let me know what code. I did a pass on all the ports as well, and they seem reasonably clean about it. The biggest problem is OImode -1, would come out as hex digits, and all the upper 0xf digits implied by sign extension would be missing. A port that cared about OImode, trivially, would fix their output routine. The debugging code has similar problems. Is this closer to something you think is in the right direction? If so, let figure out the right solution for mode_signbit_p and proceed from there. 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 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..6284d61 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -96,6 +96,9 @@ plus_constant (rtx x, HOST_WIDE_INT c) 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: @@ -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); return immed_double_const (lv, hv, VOIDmode); @@ -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 @@ -185,6 +195,7 @@ plus_constant (rtx x, HOST_WIDE_INT c) break; } + overflow: if (c != 0) x = gen_rtx_PLUS (mode, x, GEN_INT (c)); diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c index ce4eab4..37e46b1 100644 --- a/gcc/simplify-rtx.c +++ b/
Re: remove wrong code in immed_double_const
On Mar 20, 2012, at 6:55 AM, Michael Matz wrote: > Actually you did. I've tried yesterday to come up with a text that would > do the same (because I agree with you that deleting the assert changes > the spec of the function, The spec of the function is the text above the definition of the function, coupled with the information in the .texi file, would you agree? If so, could you please quote the text of the spec which would be violated by removing the assert? Could you please give a specific value with which we could talk about that shows a violation of the spec. My position is simple, the spec is what is above the definition and the .texi files, and the stuff inside the definition are interesting implementation details of that spec, which _never_ modify the spec. My position is that 0 is a value which the spec defines, and for which we assert. Please quote the line from the spec that defines what we do in that case. I've never seen anyone quote such a line. To support your position, I will insist on a direct quote from the spec. > simply because the assert _is_ part of the spec of the function), and my > attempt was _much_ worse than yours, so I didn't send it :) If you consider Eiffel, the pre and post condition on a function are indeed part of the spec of the function. But, when they are wrong and need to be fixed, you can't argue that since the spec says if I is 42, abort, then trivially, we can't fix the spec because the spec says that if I is 42, we abort. To back the position that spec must not be changed, you need to explain at least one thing for which the wrong thing will happen if the spec did change. If you want to go down that path, you will need to furnish one example where badness happens with 0, not 2, not 3, but 0. If you can't do that, you loose. If you can, love to hear it. Now, if you cite a buggy piece of software that does the wrong thing as support, I won't be swayed, I will concede the fact gcc has bugs and those bugs should be fixed, it always has, and always will. See my other post for examples of existing bugs in gcc that are not protected by the assert. I am sympathetic to preferring asserts over wrong code gen, so, I'd be willing to fix all the buggy routines or make them assert, before we loose the assert. In that case, I'd really prefer a list of concrete places to fix. An unbonded idea that the entire rest of the compiler needs fixing is, well, doesn't bode well for incremental forward progress. Given just how buggy it is, personally, I don't see the problem in just declaring that OI is completely buggy, and move on.
Re: remove wrong code in immed_double_const
On Mar 20, 2012, at 5:26 AM, Richard Sandiford wrote: > So what I was trying to say was that if we remove the assert > altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs, > we need to define what the "implicit" high-order HWIs of a > CONST_DOUBLE are, just like we already do for CONST_INT. The problem with your position is you are wrong. Let me give you an concrete example: typedef __attribute__((mode(OI))) signed int256_t; int256_t foo() { return (int256_t)0x7000 << 10; } This fails your assert you want to move down. This is _wrong_, wrong, wrong wrong. This is getting tiring. This is perfectly well defined, with no change in any definition. Would you like to try again? > If we remove the assert altogether, it very much matters what is done by that > last "*vp" line. > If Mike or anyone is up to doing that, then great. I categorically object to moving it down, it is wrong, for all the same reasons the original is wrong. > But if instead it's just a case of handling zero correctly, moving rather than > removing the assert seems safer. Sure, that fixes 1 testcase, and is monotonically better. The problem, it fixes less than 1% of the values for which we assert currently which are wrong. What is the point of bug pushing? Bug pushing is wrong. > I'm obviously not explaining this well :-) No, you are explaining your position well, it is just that your position is wrong. Your position is you want to bug push, that position is wrong. You can't win with me by having that position. The only solution is to change your position. You've done it once now, and you are moving in the right direction. You are now willing to remove the assert for 0, which you didn't want to do before. Your next stop, will be to remove the assert for (int256_t)0x7000 << 10. I will keep pushing until you relent on that point. Once you do, your position will have changed twice, mine, unchanged. I will not relent after that step, merely, I will select the next value and make you agree to it, then the next, and the next and the next, until I succeed in moving you to my original position. Now, you might be thinking that this check will protect wrong code generation in the rest of the compiler, and that may be somewhat true. A counter point would be: (int256_t)1 << 254 which would have been my next interesting value to get you to agree with me on, but, the assert you point at, doesn't prevent great badness from happening with this case. Other parts of the compiler just suck, because people didn't care enough. This I can change, but will take time. In my first patch, it will not be complete nor perfect. If you want to reject all the patches until every single last test case works, well, that isn't usually what we do. We usually approve each one as long as it is monotonically better, that's the standard by which to judge patches. Is your position, the rest of the compiler isn't perfect, so, no progress can be made in fixing it until the rest of it is perfect? If so, this is a very bad position to take. I have time now to fix and submit work to make things better. After I have things working perfectly, I may have 0 time with which to do this. I'll give you a concrete example which exactly parallels this work. There was a time when gcc didn't work on 64-bit targets. I did the first 64-bit port. It was done as a series of patches, one at a time, that pushed the compiler kicking and screaming in the right direction. That work is the basis for all the 64-bit ports today; they are now less rare than they were when I first started. gcc is reasonably good at supporting 64-bit ports. What is different today, absolutely nothing, just N. Instead of 64, it is 256, life goes on. All I can say, if you have this mistaken notion that work to make OI work should be blocked because OI doesn't work, is you are wrong. So, my question is, do you have this position?
Re: remove wrong code in immed_double_const
Hi, On Tue, 20 Mar 2012, Richard Sandiford wrote: > If Mike or anyone is up to doing that, then great. But if instead it's > just a case of handling zero correctly, moving rather than removing the > assert seems safer. > > I'm obviously not explaining this well :-) Actually you did. I've tried yesterday to come up with a text that would do the same (because I agree with you that deleting the assert changes the spec of the function, simply because the assert _is_ part of the spec of the function), and my attempt was _much_ worse than yours, so I didn't send it :) Ciao, Michael.
Re: remove wrong code in immed_double_const
On Tue, Mar 20, 2012 at 1:26 PM, Richard Sandiford wrote: > Richard Guenther writes: >>> I've no objection to moving the assert down to after the GEN_INT. >>> But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing. >>> (That is, if we remove the assert altogether, we effectively treat the >>> number as sign-extended if it happens to fit in a CONST_INT, and >>> zero-extended otherwise. >> >> Why do we treat it zero-extended otherwise? Because we use >> gen_int_mode for CONST_INTs, which sign-extends? > > Just to make sure we're not talking past each other, I meant > moving the assert to: > > /* If this integer fits in one word, return a CONST_INT. */ > [A] if ((i1 == 0 && i0 >= 0) || (i1 == ~0 && i0 < 0)) > return GEN_INT (i0); > > <---HERE---> > > /* We use VOIDmode for integers. */ > value = rtx_alloc (CONST_DOUBLE); > PUT_MODE (value, VOIDmode); > > CONST_DOUBLE_LOW (value) = i0; > CONST_DOUBLE_HIGH (value) = i1; > > for (i = 2; i < (sizeof CONST_DOUBLE_FORMAT - 1); i++) > XWINT (value, i) = 0; > > return lookup_const_double (value); > > [A] treats i0 and i1 as a sign-extended value. So if we > removed the assert (or moved it to the suggested place): > > immed_double_const (-1, -1, 4_hwi_mode) > > would create -1 in 4_hwi_mode, represented as a CONST_INT. > The three implicit high-order HWIs are -1. That's fine, > because CONST_INT has long been defined as sign-extending > rather than zero-extending. > > But if we fail the [A] test, we go on to create a CONST_DOUBLE. > The problem is that AIUI we have never defined what happens for > CONST_DOUBLE if the mode is wider than 2 HWIs. Again AIUI, > that's why the assert is there. > > This matters because of things like the handling in simplify_immed_subreg > (which, e.g., we use to generate CONST_DOUBLE pool constants, split > constant moves in lower-subreg.c, etc.). CONST_INT is already > well-defined to be a sign-extended constant, and we handle it correctly: > > switch (GET_CODE (el)) > { > case CONST_INT: > for (i = 0; > i < HOST_BITS_PER_WIDE_INT && i < elem_bitsize; > i += value_bit) > *vp++ = INTVAL (el) >> i; > /* CONST_INTs are always logically sign-extended. */ > for (; i < elem_bitsize; i += value_bit) > *vp++ = INTVAL (el) < 0 ? -1 : 0; > break; > > But because of this assert, the equivalent meaning for > CONST_DOUBLE has never been defined, and the current code > happens to zero-extend it: > > case CONST_DOUBLE: > if (GET_MODE (el) == VOIDmode) > { > /* If this triggers, someone should have generated a > CONST_INT instead. */ > gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT); > > for (i = 0; i < HOST_BITS_PER_WIDE_INT; i += value_bit) > *vp++ = CONST_DOUBLE_LOW (el) >> i; > while (i < HOST_BITS_PER_WIDE_INT * 2 && i < elem_bitsize) > { > *vp++ > = 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. */ > for (; i < elem_bitsize; i += value_bit) > *vp++ = 0; > } > > So the upshot is that: > > immed_double_const (-1, -1, 4_hwi_mode) > > sign-extends i1 (the second -1), creating (-1, -1, -1, -1). But: > > immed_double_const (0, -1, 4_hwi_mode) > > effectively (as the code falls out at the moment) zero-extends it, > creating (0, -1, 0, 0). That kind of inconsistency seems wrong. > > So what I was trying to say was that if we remove the assert > altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs, > we need to define what the "implicit" high-order HWIs of a > CONST_DOUBLE are, just like we already do for CONST_INT. > If we remove the assert altogether, it very much matters > what is done by that last "*vp" line. > > If Mike or anyone is up to doing that, then great. But if instead > it's just a case of handling zero correctly, moving rather than > removing the assert seems safer. > > I'm obviously not explaining this well :-) Ok, I see what you mean. Yes, moving the assert past the GEN_INT case (though that is specifically meant to deal with the VOIDmode case I think?) is ok. Thanks, Richard. > Richard
Re: remove wrong code in immed_double_const
Richard Guenther writes: >> I've no objection to moving the assert down to after the GEN_INT. >> But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing. >> (That is, if we remove the assert altogether, we effectively treat the >> number as sign-extended if it happens to fit in a CONST_INT, and >> zero-extended otherwise. > > Why do we treat it zero-extended otherwise? Because we use > gen_int_mode for CONST_INTs, which sign-extends? Just to make sure we're not talking past each other, I meant moving the assert to: /* If this integer fits in one word, return a CONST_INT. */ [A] if ((i1 == 0 && i0 >= 0) || (i1 == ~0 && i0 < 0)) return GEN_INT (i0); <---HERE---> /* We use VOIDmode for integers. */ value = rtx_alloc (CONST_DOUBLE); PUT_MODE (value, VOIDmode); CONST_DOUBLE_LOW (value) = i0; CONST_DOUBLE_HIGH (value) = i1; for (i = 2; i < (sizeof CONST_DOUBLE_FORMAT - 1); i++) XWINT (value, i) = 0; return lookup_const_double (value); [A] treats i0 and i1 as a sign-extended value. So if we removed the assert (or moved it to the suggested place): immed_double_const (-1, -1, 4_hwi_mode) would create -1 in 4_hwi_mode, represented as a CONST_INT. The three implicit high-order HWIs are -1. That's fine, because CONST_INT has long been defined as sign-extending rather than zero-extending. But if we fail the [A] test, we go on to create a CONST_DOUBLE. The problem is that AIUI we have never defined what happens for CONST_DOUBLE if the mode is wider than 2 HWIs. Again AIUI, that's why the assert is there. This matters because of things like the handling in simplify_immed_subreg (which, e.g., we use to generate CONST_DOUBLE pool constants, split constant moves in lower-subreg.c, etc.). CONST_INT is already well-defined to be a sign-extended constant, and we handle it correctly: switch (GET_CODE (el)) { case CONST_INT: for (i = 0; i < HOST_BITS_PER_WIDE_INT && i < elem_bitsize; i += value_bit) *vp++ = INTVAL (el) >> i; /* CONST_INTs are always logically sign-extended. */ for (; i < elem_bitsize; i += value_bit) *vp++ = INTVAL (el) < 0 ? -1 : 0; break; But because of this assert, the equivalent meaning for CONST_DOUBLE has never been defined, and the current code happens to zero-extend it: case CONST_DOUBLE: if (GET_MODE (el) == VOIDmode) { /* If this triggers, someone should have generated a CONST_INT instead. */ gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT); for (i = 0; i < HOST_BITS_PER_WIDE_INT; i += value_bit) *vp++ = CONST_DOUBLE_LOW (el) >> i; while (i < HOST_BITS_PER_WIDE_INT * 2 && i < elem_bitsize) { *vp++ = 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. */ for (; i < elem_bitsize; i += value_bit) *vp++ = 0; } So the upshot is that: immed_double_const (-1, -1, 4_hwi_mode) sign-extends i1 (the second -1), creating (-1, -1, -1, -1). But: immed_double_const (0, -1, 4_hwi_mode) effectively (as the code falls out at the moment) zero-extends it, creating (0, -1, 0, 0). That kind of inconsistency seems wrong. So what I was trying to say was that if we remove the assert altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs, we need to define what the "implicit" high-order HWIs of a CONST_DOUBLE are, just like we already do for CONST_INT. If we remove the assert altogether, it very much matters what is done by that last "*vp" line. If Mike or anyone is up to doing that, then great. But if instead it's just a case of handling zero correctly, moving rather than removing the assert seems safer. I'm obviously not explaining this well :-) Richard
Re: remove wrong code in immed_double_const
On Tue, Mar 20, 2012 at 11:49 AM, Richard Sandiford wrote: > Richard Guenther writes: >> On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump wrote: >>> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: Mike Stump writes: >> If we're going to remove the assert, we need to define stuff like >> that. > > Orthogonal. The rest of the compiler defines what happens, it either > is inconsistent, in which case it is by fiat, undefined, or it is > consistent, in which case that consistency defines it. The compiler > is free to document this in a nice way, or do, what is usually done, > which is to assume everybody just knows what it does. Anyway, my > point is, this routine doesn't define the data structure, and is > _completely_ orthogonal to your concern. It doesn't matter if it zero > extends or sign extends or is inconsistent, has bugs, doesn't have > bugs, is documented, or isn't documented. In every single one of > these cases, the code in the routine I am fixing, doesn't change. > That is _why_ it is orthogonal. If it weren't, you'd be able to state > a value for which is mattered. You can't, which is why you are wrong. > If you think you are not wrong, please state a value for which it > matters how it is defined. immed_double_const and CONST_DOUBLE are currently only defined for 2 HOST_WIDE_INTs. >>> >>> I don't happen to share your view. The routine is defined by >>> documentation. The documentation might exist in a .texi file, in this case >>> there is no texi file for immed_double_const I don't think, next up, it is >>> defined by the comments before the routine. In this case, it isn't so >>> defined. >>> >>> The current definition reads: >>> >>> /* 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. >>> Do not use this routine for non-integer modes; convert to >>> REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ >>> >>> which, is is fine, and I don't _want_ to change that definition of the >>> routine. I can't fix it, because it isn't broken. If it were, you would >>> be able to state a case where the new code behaves in a manor inconsistent >>> with the definition, since there is none you cannot state one, and this is >>> _why_ you have failed to state such a case. If you disagree, please state >>> the case. >>> >>> Now, if you review comment is, could you please update the comments in the >>> routine, I would just say, oh, sure: >>> >>> Index: emit-rtl.c >>> === >>> --- emit-rtl.c (revision 184563) >>> +++ emit-rtl.c (working copy) >>> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO >>> >>> 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) >>> { >>> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO >>> >>> if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) >>> return gen_int_mode (i0, mode); >>> - >>> - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); >>> } >>> >>> /* If this integer fits in one word, return a CONST_INT. */ >>> >>> >>> Sorry I missed it. Now, on to CONST_DOUBLE. It does appear in a texi file: >>> >>> >>> @findex const_double >>> @item (const_double:@var{m} @var{i0} @var{i1} @dots{}) >>> 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}. >>> >>> @findex CONST_DOUBLE_LOW >>> If @var{m} is @code{VOIDmode}, the bits of the value are stored in >>> @var{i0} and @var{i1}. @var{i0} is customarily accessed with the macro >>> @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}. >>> >>> >>> Here again, I don't want to change the definition. The current definition >>> applies and I am merely making the code conform to it. It says that >>> CONST_DOUBLE is used when the _value_ of the constant is too large to fit >>> into HOST_BITS_PER_WIDE_INT bits. >>> >>> So, if you disagree with me, you will
Re: remove wrong code in immed_double_const
Richard Guenther writes: > On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump wrote: >> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: >>> Mike Stump writes: > If we're going to remove the assert, we need to define stuff like > that. Orthogonal. The rest of the compiler defines what happens, it either is inconsistent, in which case it is by fiat, undefined, or it is consistent, in which case that consistency defines it. The compiler is free to document this in a nice way, or do, what is usually done, which is to assume everybody just knows what it does. Anyway, my point is, this routine doesn't define the data structure, and is _completely_ orthogonal to your concern. It doesn't matter if it zero extends or sign extends or is inconsistent, has bugs, doesn't have bugs, is documented, or isn't documented. In every single one of these cases, the code in the routine I am fixing, doesn't change. That is _why_ it is orthogonal. If it weren't, you'd be able to state a value for which is mattered. You can't, which is why you are wrong. If you think you are not wrong, please state a value for which it matters how it is defined. >>> >>> immed_double_const and CONST_DOUBLE are currently >>> only defined for 2 HOST_WIDE_INTs. >> >> I don't happen to share your view. The routine is defined by documentation. >> The documentation might exist in a .texi file, in this case there is no >> texi file for immed_double_const I don't think, next up, it is defined by >> the comments before the routine. In this case, it isn't so defined. >> >> The current definition reads: >> >> /* 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. >> Do not use this routine for non-integer modes; convert to >> REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ >> >> which, is is fine, and I don't _want_ to change that definition of the >> routine. I can't fix it, because it isn't broken. If it were, you would be >> able to state a case where the new code behaves in a manor inconsistent with >> the definition, since there is none you cannot state one, and this is _why_ >> you have failed to state such a case. If you disagree, please state the >> case. >> >> Now, if you review comment is, could you please update the comments in the >> routine, I would just say, oh, sure: >> >> Index: emit-rtl.c >> === >> --- emit-rtl.c (revision 184563) >> +++ emit-rtl.c (working copy) >> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> 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) >> { >> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) >> return gen_int_mode (i0, mode); >> - >> - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); >> } >> >> /* If this integer fits in one word, return a CONST_INT. */ >> >> >> Sorry I missed it. Now, on to CONST_DOUBLE. It does appear in a texi file: >> >> >> @findex const_double >> @item (const_double:@var{m} @var{i0} @var{i1} @dots{}) >> 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}. >> >> @findex CONST_DOUBLE_LOW >> If @var{m} is @code{VOIDmode}, the bits of the value are stored in >> @var{i0} and @var{i1}. @var{i0} is customarily accessed with the macro >> @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}. >> >> >> Here again, I don't want to change the definition. The current definition >> applies and I am merely making the code conform to it. It says that >> CONST_DOUBLE is used when the _value_ of the constant is too large to fit >> into HOST_BITS_PER_WIDE_INT bits. >> >> So, if you disagree with me, you will necessarily have to quote the >> definition you are using, explain what the words mean to you _and_ state a >> specific case in which the code post modification doe
Re: remove wrong code in immed_double_const
On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump wrote: > On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: >> Mike Stump writes: If we're going to remove the assert, we need to define stuff like that. >>> >>> Orthogonal. The rest of the compiler defines what happens, it either >>> is inconsistent, in which case it is by fiat, undefined, or it is >>> consistent, in which case that consistency defines it. The compiler >>> is free to document this in a nice way, or do, what is usually done, >>> which is to assume everybody just knows what it does. Anyway, my >>> point is, this routine doesn't define the data structure, and is >>> _completely_ orthogonal to your concern. It doesn't matter if it zero >>> extends or sign extends or is inconsistent, has bugs, doesn't have >>> bugs, is documented, or isn't documented. In every single one of >>> these cases, the code in the routine I am fixing, doesn't change. >>> That is _why_ it is orthogonal. If it weren't, you'd be able to state >>> a value for which is mattered. You can't, which is why you are wrong. >>> If you think you are not wrong, please state a value for which it >>> matters how it is defined. >> >> immed_double_const and CONST_DOUBLE are currently >> only defined for 2 HOST_WIDE_INTs. > > I don't happen to share your view. The routine is defined by documentation. > The documentation might exist in a .texi file, in this case there is no texi > file for immed_double_const I don't think, next up, it is defined by the > comments before the routine. In this case, it isn't so defined. > > The current definition reads: > > /* 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. > Do not use this routine for non-integer modes; convert to > REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ > > which, is is fine, and I don't _want_ to change that definition of the > routine. I can't fix it, because it isn't broken. If it were, you would be > able to state a case where the new code behaves in a manor inconsistent with > the definition, since there is none you cannot state one, and this is _why_ > you have failed to state such a case. If you disagree, please state the case. > > Now, if you review comment is, could you please update the comments in the > routine, I would just say, oh, sure: > > Index: emit-rtl.c > === > --- emit-rtl.c (revision 184563) > +++ emit-rtl.c (working copy) > @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO > > 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) > { > @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO > > if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) > return gen_int_mode (i0, mode); > - > - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); > } > > /* If this integer fits in one word, return a CONST_INT. */ > > > Sorry I missed it. Now, on to CONST_DOUBLE. It does appear in a texi file: > > > @findex const_double > @item (const_double:@var{m} @var{i0} @var{i1} @dots{}) > 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}. > > @findex CONST_DOUBLE_LOW > If @var{m} is @code{VOIDmode}, the bits of the value are stored in > @var{i0} and @var{i1}. @var{i0} is customarily accessed with the macro > @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}. > > > Here again, I don't want to change the definition. The current definition > applies and I am merely making the code conform to it. It says that > CONST_DOUBLE is used when the _value_ of the constant is too large to fit > into HOST_BITS_PER_WIDE_INT bits. > > So, if you disagree with me, you will necessarily have to quote the > definition you are using, explain what the words mean to you _and_ state a > specific case in which the code post modification doesn't not conform with > the existing definition. You have failed yet again to do that. > > >> So, as good functions do, immed_doub
Re: remove wrong code in immed_double_const
On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: > Mike Stump writes: >>> If we're going to remove the assert, we need to define stuff like >>> that. >> >> Orthogonal. The rest of the compiler defines what happens, it either >> is inconsistent, in which case it is by fiat, undefined, or it is >> consistent, in which case that consistency defines it. The compiler >> is free to document this in a nice way, or do, what is usually done, >> which is to assume everybody just knows what it does. Anyway, my >> point is, this routine doesn't define the data structure, and is >> _completely_ orthogonal to your concern. It doesn't matter if it zero >> extends or sign extends or is inconsistent, has bugs, doesn't have >> bugs, is documented, or isn't documented. In every single one of >> these cases, the code in the routine I am fixing, doesn't change. >> That is _why_ it is orthogonal. If it weren't, you'd be able to state >> a value for which is mattered. You can't, which is why you are wrong. >> If you think you are not wrong, please state a value for which it >> matters how it is defined. > > immed_double_const and CONST_DOUBLE are currently > only defined for 2 HOST_WIDE_INTs. I don't happen to share your view. The routine is defined by documentation. The documentation might exist in a .texi file, in this case there is no texi file for immed_double_const I don't think, next up, it is defined by the comments before the routine. In this case, it isn't so defined. The current definition reads: /* 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. Do not use this routine for non-integer modes; convert to REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE. */ which, is is fine, and I don't _want_ to change that definition of the routine. I can't fix it, because it isn't broken. If it were, you would be able to state a case where the new code behaves in a manor inconsistent with the definition, since there is none you cannot state one, and this is _why_ you have failed to state such a case. If you disagree, please state the case. Now, if you review comment is, could you please update the comments in the routine, I would just say, oh, sure: Index: emit-rtl.c === --- emit-rtl.c (revision 184563) +++ emit-rtl.c (working copy) @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO 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) { @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) return gen_int_mode (i0, mode); - - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); } /* If this integer fits in one word, return a CONST_INT. */ Sorry I missed it. Now, on to CONST_DOUBLE. It does appear in a texi file: @findex const_double @item (const_double:@var{m} @var{i0} @var{i1} @dots{}) 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}. @findex CONST_DOUBLE_LOW If @var{m} is @code{VOIDmode}, the bits of the value are stored in @var{i0} and @var{i1}. @var{i0} is customarily accessed with the macro @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}. Here again, I don't want to change the definition. The current definition applies and I am merely making the code conform to it. It says that CONST_DOUBLE is used when the _value_ of the constant is too large to fit into HOST_BITS_PER_WIDE_INT bits. So, if you disagree with me, you will necessarily have to quote the definition you are using, explain what the words mean to you _and_ state a specific case in which the code post modification doesn't not conform with the existing definition. You h
Re: remove wrong code in immed_double_const
Mike Stump writes: >> If we're going to remove the assert, we need to define stuff like >> that. > > Orthogonal. The rest of the compiler defines what happens, it either > is inconsistent, in which case it is by fiat, undefined, or it is > consistent, in which case that consistency defines it. The compiler > is free to document this in a nice way, or do, what is usually done, > which is to assume everybody just knows what it does. Anyway, my > point is, this routine doesn't define the data structure, and is > _completely_ orthogonal to your concern. It doesn't matter if it zero > extends or sign extends or is inconsistent, has bugs, doesn't have > bugs, is documented, or isn't documented. In every single one of > these cases, the code in the routine I am fixing, doesn't change. > That is _why_ it is orthogonal. If it weren't, you'd be able to state > a value for which is mattered. You can't, which is why you are wrong. > If you think you are not wrong, please state a value for which it > matters how it is defined. It isn't orthoganal. immed_double_const and CONST_DOUBLE are currently only defined for 2 HOST_WIDE_INTs. So, as good functions do, immed_double_const asserts that it is not being used out of spec. That's why the code I quoted said the filler bits for CONST_DOUBLE shouldn't matter; this assert is supposed to make sure that we never generate CONST_DOUBLEs like that. (Personally I'd have preferred it if simplify_immed_subreg asserted instead of filling with zeros.) You want to remove that restriction on immed_double_const and CONST_DOUBLE. That is, you want to change their spec. We should only do that if we define what the new semantics are. Richard
Re: remove wrong code in immed_double_const
On Mar 18, 2012, at 3:16 AM, Richard Sandiford wrote: > Mike Stump writes: >>> We currently only support constant integer >>> widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly >>> triggering if we try to build a wider constant. >> >> Can you give me a concrete example of what will fail with the constant >> 0 generated by: >> >> return GEN_INT (i0); >> >> when the mode is 2*HOST_BITS_PER_WIDE_INT? When the mode is larger than >> this? > > I think the assert is more saying that things have already gone wrong You apparently don't get it. Let me more forcefully state my position. No, things have not gone wrong. Let me repeat myself, again, 0 is perfectly representable in 0 bits, by induction it is perfectly representable in n+1 bits without loss. By induction, every integral size greater than 0 is also perfectly able to represent 0. If they had gone wrong, the testcase would not work, the code would not compile and I would not get valid code. > if we have reached this point. immed_double_const is only designed > to handle two HOST_WIDE_INTs, 0 fits into two HOST_WIDE_INTs. > so what exactly is the caller trying to do? Put 0 into a CONST_INT, with the result mode VOIDmode. > E.g. do we apply sign or zero extension to the 2 HOST_WIDE_INTs? Irrelevant. 0 has the same value, 0 or sign extended, honest. > If we're going to remove the assert, we need to define stuff like that. Orthogonal. The rest of the compiler defines what happens, it either is inconsistent, in which case it is by fiat, undefined, or it is consistent, in which case that consistency defines it. The compiler is free to document this in a nice way, or do, what is usually done, which is to assume everybody just knows what it does. Anyway, my point is, this routine doesn't define the data structure, and is _completely_ orthogonal to your concern. It doesn't matter if it zero extends or sign extends or is inconsistent, has bugs, doesn't have bugs, is documented, or isn't documented. In every single one of these cases, the code in the routine I am fixing, doesn't change. That is _why_ it is orthogonal. If it weren't, you'd be able to state a value for which is mattered. You can't, which is why you are wrong. If you think you are not wrong, please state a value for which it matters how it is defined. > All ones is a pretty common constant, so if I was going to guess, > I'd have expected we'd sign extend, Idle and irrelevant speculation. Let's not go down that path. > So immed_double_const (-1, -1, int_mode) would always be GEN_INT (-1). > But if we do that, we need to define the high HOST_WIDE_INT of a > CONST_DOUBLE to be sign-extended too. And at the moment we don't; Though this is orthogonal to the patch under consideration, I'd agree, defining the values as being sign extending seems reasonable to me. > the CONST_DOUBLE case of simplify_immed_subreg says: > > /* It shouldn't matter what's done here, so fill it with >zero. */ > for (; i < elem_bitsize; i += value_bit) > *vp++ = 0; And this would would have to be fixed, if _you_ wanted to define it as sign extending. Given this code, by fiat, it is defined to either be inconsistent or to be zero extending, presently. > From a quick grep, it looks like the case where I saw the immed_double_const > assert triggering -- the expansion of INTEGER_CST -- is the only case > where it could trigger. So I suppose the question shifts to the tree level. Again, irrelevant, let's not go there. I do not propose to define the data structure in my patch, nor to change the definition of the data structure. I merely propose to fix an obvious and trivial bug.
Re: remove wrong code in immed_double_const
Mike Stump writes: >> We currently only support constant integer >> widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly >> triggering if we try to build a wider constant. > > Can you give me a concrete example of what will fail with the constant > 0 generated by: > > return GEN_INT (i0); > > when the mode is 2*HOST_BITS_PER_WIDE_INT? When the mode is larger than this? I think the assert is more saying that things have already gone wrong if we have reached this point. immed_double_const is only designed to handle two HOST_WIDE_INTs, so what exactly is the caller trying to do? E.g. do we apply sign or zero extension to the 2 HOST_WIDE_INTs? If we're going to remove the assert, we need to define stuff like that. All ones is a pretty common constant, so if I was going to guess, I'd have expected we'd sign extend, just like we do for CONST_INT. So immed_double_const (-1, -1, int_mode) would always be GEN_INT (-1). But if we do that, we need to define the high HOST_WIDE_INT of a CONST_DOUBLE to be sign-extended too. And at the moment we don't; the CONST_DOUBLE case of simplify_immed_subreg says: /* It shouldn't matter what's done here, so fill it with zero. */ for (; i < elem_bitsize; i += value_bit) *vp++ = 0; Compare with the CONST_INT case: /* CONST_INTs are always logically sign-extended. */ for (; i < elem_bitsize; i += value_bit) *vp++ = INTVAL (el) < 0 ? -1 : 0; So... > See, we already shorten the width of wide constants and expect that to > work. This is just another instance of shortening. Now, just to see > what lurks in there, I when through 100% of the uses of CONST_DOUBLE > in *.c to get a feel for what might go wrong, the code is as benign as > I expected, every instance I saw, looked reasonably safe. It doesn't > get any better than this. ...I'm not sure about. >From a quick grep, it looks like the case where I saw the immed_double_const assert triggering -- the expansion of INTEGER_CST -- is the only case where it could trigger. So I suppose the question shifts to the tree level. Are callers to build_int_cst_wide and double_int_to_tree already expected to check that constants wider than a double_int would not be truncated? If so, then great. And if so, what are the rules there regarding sign vs. zero extension beyond the high HOST_WIDE_INT? >> FWIW, here's another case where this came up: >> >>http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html > > Yes, and surprisingly, or not it is the exact same case as I can into. I'd expect it to be a slightly different case. The original testcase was a union constructor that was unnecessarily initialised to zero. That has since been fixed. Richard
Re: remove wrong code in immed_double_const
On Mar 17, 2012, at 12:37 AM, Richard Sandiford wrote: > Mike Stump writes: >> This removes some wrong code. >> >> Ok? >> >> Index: gcc/emit-rtl.c >> === >> --- gcc/emit-rtl.c (revision 184563) >> +++ gcc/emit-rtl.c (working copy) >> @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) >>return gen_int_mode (i0, mode); >> - >> - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); >> } >> >> /* If this integer fits in one word, return a CONST_INT. */ > > Is this because you have an integer mode between HOST_BITS_PER_WIDE_INT and > 2 * HOST_BITS_PER_WIDE_INT? Yes, I have those, but, that wasn't the testcase I had in mind. > Or is this because you have an integer mode wider than > 2*OST_BITS_PER_WIDE_INT? Yes. > We currently only support constant integer > widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly > triggering if we try to build a wider constant. Can you give me a concrete example of what will fail with the constant 0 generated by: return GEN_INT (i0); when the mode is 2*HOST_BITS_PER_WIDE_INT? When the mode is larger than this? If you cannot, can you refer me to documentation where this is discussed? If not, how about code? What I am seeing is that it works and generates correct code without the assert. My contention is that any code that fails to work, is buggy, and should be fixed, and once fixed, then there is no code that doesn't work, and hence the assert is wrong. If you want to argue that the code that fails, isn't buggy, go ahead, I'd love to hear it. See, we already shorten the width of wide constants and expect that to work. This is just another instance of shortening. Now, just to see what lurks in there, I when through 100% of the uses of CONST_DOUBLE in *.c to get a feel for what might go wrong, the code is as benign as I expected, every instance I saw, looked reasonably safe. It doesn't get any better than this. > Obviously it'd be nice Yes, but that is quite a lot of work. It also happens to be orthogonal to the immediate issue at hand. > if we supported arbitrary widths, e.g. by replacing CONST_INT and > CONST_DOUBLE with a single n-HOST_WIDE_INT rtx (and immed_double_const > with some kind of nary builder, etc.). It won't be easy though. > > Removing the assert seems like papering over the problem. Do you have an example of a failure? Hint, without a failure, there is no bug, I cannot fix a bug, when there is no bug. > FWIW, here's another case where this came up: > >http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html Yes, and surprisingly, or not it is the exact same case as I can into. Notice nowhere in that bug or thread, is _any_ detailed discussed as to why that would be a bad fix. So, I'm looking for approval, or a concrete reason why it is a bad idea.
Re: remove wrong code in immed_double_const
Mike Stump writes: > This removes some wrong code. > > Ok? > > Index: gcc/emit-rtl.c > === > --- gcc/emit-rtl.c (revision 184563) > +++ gcc/emit-rtl.c (working copy) > @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO > >if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) > return gen_int_mode (i0, mode); > - > - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); > } > >/* If this integer fits in one word, return a CONST_INT. */ Is this because you have an integer mode between HOST_BITS_PER_WIDE_INT and 2 * HOST_BITS_PER_WIDE_INT? (I.e. a partial one?) If so, I can see an argument for changing the "==" to "<=", although we'd need to think carefully about what CONST_DOUBLE means in that case. (Endianness, etc.) Or is this because you have an integer mode wider than 2*OST_BITS_PER_WIDE_INT? We currently only support constant integer widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly triggering if we try to build a wider constant. Obviously it'd be nice if we supported arbitrary widths, e.g. by replacing CONST_INT and CONST_DOUBLE with a single n-HOST_WIDE_INT rtx (and immed_double_const with some kind of nary builder, etc.). It won't be easy though. Removing the assert seems like papering over the problem. FWIW, here's another case where this came up: http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html Richard
Re: remove wrong code in immed_double_const
On Mar 16, 2012, at 3:03 PM, Steven Bosscher wrote: > On Fri, Mar 16, 2012 at 10:54 PM, Mike Stump wrote: >> This removes some wrong code. >> >> Ok? > > ChangeLog is missing. * emit-rtl.c (immed_double_const): Remove bogus assert. > Tested how? Compiles a user program for my port. > And why is this wrong anyway? It is wrong because it can assert. The code can generate a constant for every inbound value that it asserts for, so trivially, it is wrong. If it were correct, it would not assert for values it can handle. For it to be correct, there would have to exist a value for which the code fails. The value that failed for me was 0, not a terribly uncommon value.
Re: remove wrong code in immed_double_const
On Fri, Mar 16, 2012 at 10:54 PM, Mike Stump wrote: > This removes some wrong code. > > Ok? ChangeLog is missing. Tested how? And why is this wrong anyway? Ciao! Steven > Index: gcc/emit-rtl.c > === > --- gcc/emit-rtl.c (revision 184563) > +++ gcc/emit-rtl.c (working copy) > @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO > > if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) > return gen_int_mode (i0, mode); > - > - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); > } > > /* If this integer fits in one word, return a CONST_INT. */ >