Message ID | 57F7AC64.900@foss.arm.com |
---|---|
State | New |
Headers | show |
On 10/07/2016 04:08 PM, Kyrill Tkachov wrote: > > * simplify-rtx.c (simplify_immed_subreg): Zero-initialize tmp array > before merging in bytes to pass down to real_from_target. Ok. Bernd
On Fri, Oct 7, 2016 at 7:08 AM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > Hi all, > > I've encountered another wrong-code bug with the store merging pass. This > time it's in RTL. > The test gcc.target/aarch64/aapcs64/test_27.c on aarch64 merges a few __fp16 > values at GIMPLE level but > during RTL dse1 one of the constants generated gets wrongly misinterpreted > from HImode to HFmode > by simplify_immed_subreg. The HFmode value it ends up producing is > completely bogus. > > By stepping through the code with GDB the problem is in the hunk touched by > this patch when it > fills in an array of longs with the value bytes before passing it down to > real_from_target. > It fills in the array by orring in each byte. > > However, the array it declared to use for this doesn not get properly > zero-initialised for modes > less that 32 bits wide (HFmode in this case). The fix in this patch is to > just use an array initialiser > to zero it out. This makes the failure go away. > > Bootstrapped and tested on aarch64, x86_64. > > Ok for trunk? Even though this has been approved I think it is best to do write this code as this: long tmp[MAX_BITSIZE_MODE_ANY_MODE / 32]; /* real_from_target wants its input in words affected by FLOAT_WORDS_BIG_ENDIAN. However, we ignore this, and use WORDS_BIG_ENDIAN instead; see the documentation of SUBREG in rtl.texi. */ memset (tmp, 0, sizeof(tmp)); Thanks, Andrew Pinski > Thanks, > Kyrill > > 2016-10-06 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * simplify-rtx.c (simplify_immed_subreg): Zero-initialize tmp array > before merging in bytes to pass down to real_from_target.
On Fri, Oct 7, 2016 at 10:55 AM, Andrew Pinski <pinskia@gmail.com> wrote: > On Fri, Oct 7, 2016 at 7:08 AM, Kyrill Tkachov > <kyrylo.tkachov@foss.arm.com> wrote: >> Hi all, >> >> I've encountered another wrong-code bug with the store merging pass. This >> time it's in RTL. >> The test gcc.target/aarch64/aapcs64/test_27.c on aarch64 merges a few __fp16 >> values at GIMPLE level but >> during RTL dse1 one of the constants generated gets wrongly misinterpreted >> from HImode to HFmode >> by simplify_immed_subreg. The HFmode value it ends up producing is >> completely bogus. >> >> By stepping through the code with GDB the problem is in the hunk touched by >> this patch when it >> fills in an array of longs with the value bytes before passing it down to >> real_from_target. >> It fills in the array by orring in each byte. >> >> However, the array it declared to use for this doesn not get properly >> zero-initialised for modes >> less that 32 bits wide (HFmode in this case). The fix in this patch is to >> just use an array initialiser >> to zero it out. This makes the failure go away. >> >> Bootstrapped and tested on aarch64, x86_64. >> >> Ok for trunk? > > Even though this has been approved I think it is best to do write this > code as this: > long tmp[MAX_BITSIZE_MODE_ANY_MODE / 32]; > /* real_from_target wants its input in words affected by > FLOAT_WORDS_BIG_ENDIAN. However, we ignore this, > and use WORDS_BIG_ENDIAN instead; see the documentation > of SUBREG in rtl.texi. */ > memset (tmp, 0, sizeof(tmp)); Also the / 32 should be changed to / (sizeof(long) * BITS_PER_CHAR) but that was there before hand. THanks, Andrew > > Thanks, > Andrew Pinski > > >> Thanks, >> Kyrill >> >> 2016-10-06 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> * simplify-rtx.c (simplify_immed_subreg): Zero-initialize tmp array >> before merging in bytes to pass down to real_from_target.
On 07/10/16 18:56, Andrew Pinski wrote: > On Fri, Oct 7, 2016 at 10:55 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> On Fri, Oct 7, 2016 at 7:08 AM, Kyrill Tkachov >> <kyrylo.tkachov@foss.arm.com> wrote: >>> Hi all, >>> >>> I've encountered another wrong-code bug with the store merging pass. This >>> time it's in RTL. >>> The test gcc.target/aarch64/aapcs64/test_27.c on aarch64 merges a few __fp16 >>> values at GIMPLE level but >>> during RTL dse1 one of the constants generated gets wrongly misinterpreted >>> from HImode to HFmode >>> by simplify_immed_subreg. The HFmode value it ends up producing is >>> completely bogus. >>> >>> By stepping through the code with GDB the problem is in the hunk touched by >>> this patch when it >>> fills in an array of longs with the value bytes before passing it down to >>> real_from_target. >>> It fills in the array by orring in each byte. >>> >>> However, the array it declared to use for this doesn not get properly >>> zero-initialised for modes >>> less that 32 bits wide (HFmode in this case). The fix in this patch is to >>> just use an array initialiser >>> to zero it out. This makes the failure go away. >>> >>> Bootstrapped and tested on aarch64, x86_64. >>> >>> Ok for trunk? >> Even though this has been approved I think it is best to do write this >> code as this: >> long tmp[MAX_BITSIZE_MODE_ANY_MODE / 32]; >> /* real_from_target wants its input in words affected by >> FLOAT_WORDS_BIG_ENDIAN. However, we ignore this, >> and use WORDS_BIG_ENDIAN instead; see the documentation >> of SUBREG in rtl.texi. */ >> memset (tmp, 0, sizeof(tmp)); > > Also the / 32 should be changed to / (sizeof(long) * BITS_PER_CHAR) > but that was there before hand. Agreed about this, but I'm not sure about the memset. long tmp[<size>] = { 0 }; looks shorter and cleaner to me and is guaranteed to do the right thing as well, no? Thanks, Kyrill > THanks, > Andrew > >> Thanks, >> Andrew Pinski >> >> >>> Thanks, >>> Kyrill >>> >>> 2016-10-06 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> * simplify-rtx.c (simplify_immed_subreg): Zero-initialize tmp array >>> before merging in bytes to pass down to real_from_target.
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c index 4bb16f896f89b06612e923a0b8db8e074b8a734c..9e7376c9f2b967d96b3195a6d9de57a543644d10 100644 --- a/gcc/simplify-rtx.c +++ b/gcc/simplify-rtx.c @@ -5906,14 +5906,12 @@ simplify_immed_subreg (machine_mode outermode, rtx op, case MODE_DECIMAL_FLOAT: { REAL_VALUE_TYPE r; - long tmp[MAX_BITSIZE_MODE_ANY_MODE / 32]; + long tmp[MAX_BITSIZE_MODE_ANY_MODE / 32] = { 0 }; /* real_from_target wants its input in words affected by FLOAT_WORDS_BIG_ENDIAN. However, we ignore this, and use WORDS_BIG_ENDIAN instead; see the documentation of SUBREG in rtl.texi. */ - for (i = 0; i < max_bitsize / 32; i++) - tmp[i] = 0; for (i = 0; i < elem_bitsize; i += value_bit) { int ibase;