| Message ID | 87zjni745b.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
Nick Clifton <nickc@redhat.com> wrote: >Hi Guys, > > I have tracked down a bug reported against the MSP430 (PR > target/59613) which turns out to be a generic problem. The function > get_mode_bounds() in stor-layout.c computes the minimum and maximum > values for a given mode, but it uses the bitsize of the mode not the > precision. This is incorrect if the two values are different, (which > can happen when the target is using partial integer modes), since > values outside of the precision cannot be stored in the mode, no > matter how many bits the mode uses. > > The result, for the MSP430 in LARGE mode, is that calling > get_mode_bounds() on PSImode returns a maximum value of -1 instead of > 1^20. Note - what happens is that get_mode_bounds computes the > maximum as (1 << 31) - 1 and then calls gen_int_mode to create an RTX > for this value. But gen_int_mode calls trunc_int_for_mode which uses > GET_MODE_PRECISION to determine the sign bits and these bits overwrite > some of the zero bits in (1 << 31) - 1 changing it into -1. > > The result of this behaviour is the bug that I was tracking down. > simplify_const_relational_operation() was erroneously discarding a > comparison of a pointer against zero, because get_mode_bounds told it > that the pointer could only have values between 0 and -1... > > The proposed patch is simple - use the mode's precision and not its > bitsize to compute the bounds. This seems like an obvious fix, but > I am not familiar with all of the uses of get_mode_bounds() so maybe > there is a situation where using the bitsize is correct. > > There were no regressions and several fixed test cases with the > msp430-elf toolchain that I was using, and no regressions with an > i686-pc-linux-gnu toolchain. > > OK to apply ? Ok without the comment (this is obvious) Thanks, Richard. >Cheers > Nick > >PS. Just found out that the same patch was created by Peter Bigot for >a >different PR: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52856 > > >gcc/ChangeLog >2013-12-30 Nick Clifton <nickc@redhat.com> > > PR target/59631 > * stor-layout.c (get_mode_bounds): Use GET_MODE_PRECISION instead > of GET_MODE_BITSIZE. > >Index: stor-layout.c >=================================================================== >--- stor-layout.c (revision 206241) >+++ stor-layout.c (working copy) >@@ -2816,7 +2816,8 @@ > enum machine_mode target_mode, > rtx *mmin, rtx *mmax) > { >- unsigned size = GET_MODE_BITSIZE (mode); >+ /* PR 59613: Use the precision of the mode, not its bitsize. */ >+ unsigned size = GET_MODE_PRECISION (mode); > unsigned HOST_WIDE_INT min_val, max_val; > > gcc_assert (size <= HOST_BITS_PER_WIDE_INT);
> Ok without the comment (this is obvious)
And with an appropriate name for the variable, typically "prec".
Richard Biener <richard.guenther@gmail.com> writes: > Nick Clifton <nickc@redhat.com> wrote: >>Hi Guys, >> >> I have tracked down a bug reported against the MSP430 (PR >> target/59613) which turns out to be a generic problem. The function >> get_mode_bounds() in stor-layout.c computes the minimum and maximum >> values for a given mode, but it uses the bitsize of the mode not the >> precision. This is incorrect if the two values are different, (which >> can happen when the target is using partial integer modes), since >> values outside of the precision cannot be stored in the mode, no >> matter how many bits the mode uses. >> >> The result, for the MSP430 in LARGE mode, is that calling >> get_mode_bounds() on PSImode returns a maximum value of -1 instead of >> 1^20. Note - what happens is that get_mode_bounds computes the >> maximum as (1 << 31) - 1 and then calls gen_int_mode to create an RTX >> for this value. But gen_int_mode calls trunc_int_for_mode which uses >> GET_MODE_PRECISION to determine the sign bits and these bits overwrite >> some of the zero bits in (1 << 31) - 1 changing it into -1. >> >> The result of this behaviour is the bug that I was tracking down. >> simplify_const_relational_operation() was erroneously discarding a >> comparison of a pointer against zero, because get_mode_bounds told it >> that the pointer could only have values between 0 and -1... >> >> The proposed patch is simple - use the mode's precision and not its >> bitsize to compute the bounds. This seems like an obvious fix, but >> I am not familiar with all of the uses of get_mode_bounds() so maybe >> there is a situation where using the bitsize is correct. >> >> There were no regressions and several fixed test cases with the >> msp430-elf toolchain that I was using, and no regressions with an >> i686-pc-linux-gnu toolchain. >> >> OK to apply ? > > Ok without the comment (this is obvious) This completely breaks ia64, see PR59649. Andreas.
Index: stor-layout.c =================================================================== --- stor-layout.c (revision 206241) +++ stor-layout.c (working copy) @@ -2816,7 +2816,8 @@ enum machine_mode target_mode, rtx *mmin, rtx *mmax) { - unsigned size = GET_MODE_BITSIZE (mode); + /* PR 59613: Use the precision of the mode, not its bitsize. */ + unsigned size = GET_MODE_PRECISION (mode); unsigned HOST_WIDE_INT min_val, max_val; gcc_assert (size <= HOST_BITS_PER_WIDE_INT);