| Message ID | 559BC80B.4060608@arm.com |
|---|---|
| State | New |
| Headers | show |
On 07/07/2015 06:37 AM, Alan Lawrence wrote: > As per https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01346.html. Fixes > FAIL of advsimd-intrinsics vcreate.c on aarch64_be-none-elf from > previous patch. > > 15_native_interpret_real.patch > > > commit e2e7ca148960a82fc88128820f17e7cbd14173cb > Author: Alan Lawrence<alan.lawrence@arm.com> > Date: Thu Apr 9 10:54:40 2015 +0100 > > Fix native_interpret_real for HFmode floats on Bigendian with UNITS_PER_WORD>=4 > > (with missing space) OK with ChangeLog in proper form. jeff
On Wed, Jul 8, 2015 at 12:07 AM, Jeff Law <law@redhat.com> wrote: > On 07/07/2015 06:37 AM, Alan Lawrence wrote: >> >> As per https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01346.html. Fixes >> FAIL of advsimd-intrinsics vcreate.c on aarch64_be-none-elf from >> previous patch. >> >> 15_native_interpret_real.patch >> >> >> commit e2e7ca148960a82fc88128820f17e7cbd14173cb >> Author: Alan Lawrence<alan.lawrence@arm.com> >> Date: Thu Apr 9 10:54:40 2015 +0100 >> >> Fix native_interpret_real for HFmode floats on Bigendian with >> UNITS_PER_WORD>=4 >> >> (with missing space) > > OK with ChangeLog in proper form. Err - but now offset can become negative? Shouldn't it rather error out before as it requires at least 4 bytes for big-endian? That said - the whole thing looks it doesn't expect GET_MODE_SIZE < 4 and your "fix" is just very obfuscated (if it really is a fix). So, please cleanup the thing properly instead or at least add a big fat comment. There is the magic '3' in the line following yours as well. Thanks, Richard. > jeff >
Richard Biener wrote: > On Wed, Jul 8, 2015 at 12:07 AM, Jeff Law <law@redhat.com> wrote: >> On 07/07/2015 06:37 AM, Alan Lawrence wrote: [snip] >>> Fix native_interpret_real for HFmode floats on Bigendian with >>> UNITS_PER_WORD>=4 >>> >>> (with missing space) >> OK with ChangeLog in proper form. > > Err - but now offset can become negative? Shouldn't it rather error out > before as it requires at least 4 bytes for big-endian? I don't think the offset can ever be negative; my reasoning is: total_bytes = GET_MODE_SIZE (TYPE_MODE (type)) [set just before loop] bitpos < total_bytes * BITS_PER_UNIT [condition of for loop] byte = (bitpos / BITS_PER_UNIT) & 3 [first statement inside for loop] ==> byte < 3 && byte < total_bytes ==> byte < MIN (3, total_bytes) ==> byte <= MIN (3, total_bytes - 1) > That said - the whole thing looks it doesn't expect GET_MODE_SIZE < 4 > and your "fix" is just very obfuscated (if it really is a fix). > > So, please cleanup the thing properly instead or at least add a big fat > comment. There is the magic '3' in the line following yours as well. Ok, I'll try to cleanup, I admit I'm not sure what all that code does (particularly if UNITS_PER_WORD < 4 !)... --Alan
On Wed, Jul 8, 2015 at 12:51 PM, Alan Lawrence <alan.lawrence@arm.com> wrote: > Richard Biener wrote: >> >> On Wed, Jul 8, 2015 at 12:07 AM, Jeff Law <law@redhat.com> wrote: >>> >>> On 07/07/2015 06:37 AM, Alan Lawrence wrote: > > [snip] >>>> >>>> Fix native_interpret_real for HFmode floats on Bigendian with >>>> UNITS_PER_WORD>=4 >>>> >>>> (with missing space) >>> >>> OK with ChangeLog in proper form. >> >> >> Err - but now offset can become negative? Shouldn't it rather error out >> before as it requires at least 4 bytes for big-endian? > > > I don't think the offset can ever be negative; my reasoning is: > > total_bytes = GET_MODE_SIZE (TYPE_MODE (type)) [set just before loop] > bitpos < total_bytes * BITS_PER_UNIT [condition of for loop] > byte = (bitpos / BITS_PER_UNIT) & 3 [first statement inside for loop] > > ==> byte < 3 && byte < total_bytes > ==> byte < MIN (3, total_bytes) > ==> byte <= MIN (3, total_bytes - 1) > >> That said - the whole thing looks it doesn't expect GET_MODE_SIZE < 4 >> and your "fix" is just very obfuscated (if it really is a fix). >> >> So, please cleanup the thing properly instead or at least add a big fat >> comment. There is the magic '3' in the line following yours as well. > > > Ok, I'll try to cleanup, I admit I'm not sure what all that code does > (particularly if UNITS_PER_WORD < 4 !)... Yeah, me neither - but I'm trying to at least make sure it doesn't get worse ;) Richard. > --Alan >
On 07/08/2015 03:43 AM, Richard Biener wrote: > On Wed, Jul 8, 2015 at 12:07 AM, Jeff Law <law@redhat.com> wrote: >> On 07/07/2015 06:37 AM, Alan Lawrence wrote: >>> >>> As per https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01346.html. Fixes >>> FAIL of advsimd-intrinsics vcreate.c on aarch64_be-none-elf from >>> previous patch. >>> >>> 15_native_interpret_real.patch >>> >>> >>> commit e2e7ca148960a82fc88128820f17e7cbd14173cb >>> Author: Alan Lawrence<alan.lawrence@arm.com> >>> Date: Thu Apr 9 10:54:40 2015 +0100 >>> >>> Fix native_interpret_real for HFmode floats on Bigendian with >>> UNITS_PER_WORD>=4 >>> >>> (with missing space) >> >> OK with ChangeLog in proper form. > > Err - but now offset can become negative? Shouldn't it rather error out > before as it requires at least 4 bytes for big-endian? I managed to convince myself the value wouldn't be negative when reviewing. > > That said - the whole thing looks it doesn't expect GET_MODE_SIZE < 4 > and your "fix" is just very obfuscated (if it really is a fix). While I couldn't convince myself the function as a whole was prepared for smaller objects, I don't think Alan's patch made things worse. One could argue the whole bloody thing ought to be rewritten though. I'd also managed to convince myself the other instances of "3" weren't problematical. jeff
commit e2e7ca148960a82fc88128820f17e7cbd14173cb
Author: Alan Lawrence <alan.lawrence@arm.com>
Date: Thu Apr 9 10:54:40 2015 +0100
Fix native_interpret_real for HFmode floats on Bigendian with UNITS_PER_WORD>=4
(with missing space)
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index e61d946..15a10f0 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -7622,7 +7622,7 @@ native_interpret_real (tree type, const unsigned char *ptr, int len)
offset += byte % UNITS_PER_WORD;
}
else
- offset = BYTES_BIG_ENDIAN ? 3 - byte : byte;
+ offset = BYTES_BIG_ENDIAN ? MIN (3, total_bytes - 1) - byte : byte;
value = ptr[offset + ((bitpos / BITS_PER_UNIT) & ~3)];
tmp[bitpos / 32] |= (unsigned long)value << (bitpos & 31);