| 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;
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? 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.