| Message ID | 0A5CBD0C-FC94-4637-B230-1A83372DE91A@comcast.net |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikestump@comcast.net> wrote: > On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: >> Mike Stump <mikestump@comcast.net> 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_double_const asserts that it is not being used out of spec. > > This does not follow from the definition. 0 is a value that fits into HOST_BITS_PER_WIDE_INT bits. It is representable in 0 bits. HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST_BITS_PER_WIDE_INT bits. > >> 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. > > You're assuming a definition for CONST_DOUBLE that only exists in your mind, instead, please refer to the actual definition in the .texi file. Btw, I agree with Mike here (quite obvious if you followed the old e-mail thread). But as there is some disagreement here I leave approval of the patch with the comment change to someone to break that tie ;) Thanks, Richard.
Richard Guenther <richard.guenther@gmail.com> writes: > On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikestump@comcast.net> wrote: >> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: >>> Mike Stump <mikestump@comcast.net> 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_double_const asserts that it is not being used out of spec. >> >> This does not follow from the definition. 0 is a value that fits into HOST_BITS_PER_WIDE_INT bits. It is representable in 0 bits. HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST_BITS_PER_WIDE_INT bits. >> >>> 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. >> >> You're assuming a definition for CONST_DOUBLE that only exists in your mind, instead, please refer to the actual definition in the .texi file. > > Btw, I agree with Mike here (quite obvious if you followed the old > e-mail thread). 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. That kind of inconsistency seems wrong, and could turn what is now an ICE into a wrong code bug.) > But as there is some disagreement here I leave approval of the patch with the > comment change to someone to break that tie ;) No need for that. Clearly it's just me :-) Please go ahead and approve whatever you think is right. Richard
On Tue, Mar 20, 2012 at 11:49 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Guenther <richard.guenther@gmail.com> writes: >> On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikestump@comcast.net> wrote: >>> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote: >>>> Mike Stump <mikestump@comcast.net> 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_double_const asserts that it is not being used out of spec. >>> >>> This does not follow from the definition. 0 is a value that fits into HOST_BITS_PER_WIDE_INT bits. It is representable in 0 bits. HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST_BITS_PER_WIDE_INT bits. >>> >>>> 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. >>> >>> You're assuming a definition for CONST_DOUBLE that only exists in your mind, instead, please refer to the actual definition in the .texi file. >> >> Btw, I agree with Mike here (quite obvious if you followed the old >> e-mail thread). > > 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? Yes, if we'd have a partial integer mode we would not properly truncate/extend the double-int input. OTOH all double-ints should come from trees where the constants are already properly extended (not sure why we forcefully sign-extend CONST_INTs for modes that are smaller than a HWI). > That kind of inconsistency seems wrong, > and could turn what is now an ICE into a wrong code bug.) > >> But as there is some disagreement here I leave approval of the patch with the >> comment change to someone to break that tie ;) > > No need for that. Clearly it's just me :-) Please go ahead and approve > whatever you think is right. Well, even if I cannot make sense of the assert (and the assert does not change the above observation of inconsistency wrt extending partial integer modes) I'm not familiar enough with RTL to be confident to approve this change ;) Richard. > Richard
Richard Guenther <richard.guenther@gmail.com> 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
On Tue, Mar 20, 2012 at 1:26 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Guenther <richard.guenther@gmail.com> 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
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.
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)0x7000000000000000 << 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)0x7000000000000000 << 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?
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.
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. did so, I just repeated him. Ciao, Michael.
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.
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.
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.
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. */