| Message ID | b6760321e87572ff1ec815b7f3a2af1ef8394648.camel@t-online.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RFH,libgcc] fp-bit bit ordering (PR 78804) | expand |
On 9/28/19 8:14 PM, Oleg Endo wrote: > Hi, > > I've been dragging this patch along with me for a while. > At the moment, I don't have the resources to fully test it as requested > by Ian in the PR discussion. > > So I would like to ask for general comments on this one and hope that > folks with bigger automated test setups can run the patch through their > machinery for little endian targets. > > > Summary of the story: > > I've noticed this issue on the RX on GCC 6, but it seems it's been > there forever. > > On RX, fp-bit is used for software floating point emulation. The RX > target also uses "MS bit-field" layout by default. This means that > code like > > struct > { > fractype fraction:FRACBITS __attribute__ ((packed)); > unsigned int exp:EXPBITS __attribute__ ((packed)); > unsigned int sign:1 __attribute__ ((packed)); > } bits; > > will result in sizeof (bits) != 8 > > For some reason, this bit-field style declaration is used only for > FLOAT_BIT_ORDER_MISMATCH, which generally seems to be set for little > endian targets. In other cases (i.e. big endian) open coded bit field > extraction and packing is used on the base integer type, like > > fraction = src->value_raw & ((((fractype)1) << FRACBITS) - 1); > exp = ((int)(src->value_raw >> FRACBITS)) & ((1 << EXPBITS) - 1); > sign = ((int)(src->value_raw >> (FRACBITS + EXPBITS))) & 1; > > This works of course regardless of the bit-field packing layout of the > target. > > Joseph suggested to pack the struct bit, which would fix the issue. > https://gcc.gnu.org/ml/gcc-bugs/2017-08/msg01651.html > > However, I would like to propose to remove the special case of > FLOAT_BIT_ORDER_MISMATCH altogether as in the attached patch. > > Any comments? > > Cheers, > Oleg > > > > libgcc/ChangeLog > > PR libgcc/77804 > * fp-bit.h: Remove FLOAT_BIT_ORDER_MISMATCH. > * fp-bit.c (pack_d, unpack_d): Remove special cases for > FLOAT_BIT_ORDER_MISMATCH. > * config/arc/t-arc: Remove FLOAT_BIT_ORDER_MISMATCH. > So the ask is to just test this on some LE targets? I can do that :-) I'll throw it in. Analysis will be slightly more difficult than usual as we've got some fallout from Richard S's work, but it's certainly do-able. Jeff ps. ANd yes, I've got a request to the build farm folks to get a jenkins instance on the build farm. Once that's in place I can have my tester start publishing results that everyone can see.
On Tue, 2019-10-01 at 14:21 -0600, Jeff Law wrote: > > So the ask is to just test this on some LE targets? I can do that :-) > > I'll throw it in. Analysis will be slightly more difficult than > usual > as we've got some fallout from Richard S's work, but it's certainly > do-able. Thanks a lot! > ps. ANd yes, I've got a request to the build farm folks to get a > jenkins instance on the build farm. Once that's in place I can have > my tester start publishing results that everyone can see. Sounds great. Would it be possible for other people to give the auto tester patches for testing and get the results back from it? Or something like that? Cheers, Oleg
On 10/2/19 10:51 AM, Oleg Endo wrote: > On Tue, 2019-10-01 at 14:21 -0600, Jeff Law wrote: >> >> So the ask is to just test this on some LE targets? I can do that :-) >> >> I'll throw it in. Analysis will be slightly more difficult than >> usual >> as we've got some fallout from Richard S's work, but it's certainly >> do-able. > > Thanks a lot! > > >> ps. ANd yes, I've got a request to the build farm folks to get a >> jenkins instance on the build farm. Once that's in place I can have >> my tester start publishing results that everyone can see. > > Sounds great. Would it be possible for other people to give the auto > tester patches for testing and get the results back from it? Or > something like that? That's certainly the medium term plan. THe initial first step is to proxy the results from the Jenkins instance on my laptop and Red Hat's Toronto office to a public instance running in the GCC farm. That way everyone can see the results & log files even if they can't (yet) start runs or submit patches for subsequent runs. jeff
On 9/28/19 8:14 PM, Oleg Endo wrote: > Hi, > > I've been dragging this patch along with me for a while. > At the moment, I don't have the resources to fully test it as requested > by Ian in the PR discussion. > > So I would like to ask for general comments on this one and hope that > folks with bigger automated test setups can run the patch through their > machinery for little endian targets. > > > Summary of the story: > > I've noticed this issue on the RX on GCC 6, but it seems it's been > there forever. > > On RX, fp-bit is used for software floating point emulation. The RX > target also uses "MS bit-field" layout by default. This means that > code like > > struct > { > fractype fraction:FRACBITS __attribute__ ((packed)); > unsigned int exp:EXPBITS __attribute__ ((packed)); > unsigned int sign:1 __attribute__ ((packed)); > } bits; > > will result in sizeof (bits) != 8 > > For some reason, this bit-field style declaration is used only for > FLOAT_BIT_ORDER_MISMATCH, which generally seems to be set for little > endian targets. In other cases (i.e. big endian) open coded bit field > extraction and packing is used on the base integer type, like > > fraction = src->value_raw & ((((fractype)1) << FRACBITS) - 1); > exp = ((int)(src->value_raw >> FRACBITS)) & ((1 << EXPBITS) - 1); > sign = ((int)(src->value_raw >> (FRACBITS + EXPBITS))) & 1; > > This works of course regardless of the bit-field packing layout of the > target. > > Joseph suggested to pack the struct bit, which would fix the issue. > https://gcc.gnu.org/ml/gcc-bugs/2017-08/msg01651.html > > However, I would like to propose to remove the special case of > FLOAT_BIT_ORDER_MISMATCH altogether as in the attached patch. > > Any comments? So probably the most interesting target for this test is v850-elf as it's got a reasonably well functioning simulator, hard and soft FP targets, little endian, and I'm familiar with its current set of failures. I can confirm that your patch makes no difference in the test results (which includes execution results). In fact, there haven't been any problems on any target in my tester that I can tie back to this change. At this point I'd say let's go for it. jeff > > Cheers, > Oleg > > > > libgcc/ChangeLog > > PR libgcc/77804 > * fp-bit.h: Remove FLOAT_BIT_ORDER_MISMATCH. > * fp-bit.c (pack_d, unpack_d): Remove special cases for > FLOAT_BIT_ORDER_MISMATCH. > * config/arc/t-arc: Remove FLOAT_BIT_ORDER_MISMATCH. >
On Thu, 2019-10-03 at 19:34 -0600, Jeff Law wrote: > > So probably the most interesting target for this test is v850-elf as > it's got a reasonably well functioning simulator, hard and soft FP > targets, little endian, and I'm familiar with its current set of > failures. > > I can confirm that your patch makes no difference in the test results > (which includes execution results). > > In fact, there haven't been any problems on any target in my tester > that > I can tie back to this change. > > At this point I'd say let's go for it. > Thanks, Jeff. I'll commit it to trunk if there are no further objections some time next week. It's a latent issue, not a regression. OK for the open branches, too? Any opinions on that? Cheers, Oleg
On Fri, 2019-10-11 at 23:27 +0900, Oleg Endo wrote: > On Thu, 2019-10-03 at 19:34 -0600, Jeff Law wrote: > > > > So probably the most interesting target for this test is v850-elf > > as > > it's got a reasonably well functioning simulator, hard and soft FP > > targets, little endian, and I'm familiar with its current set of > > failures. > > > > I can confirm that your patch makes no difference in the test > > results > > (which includes execution results). > > > > In fact, there haven't been any problems on any target in my tester > > that > > I can tie back to this change. > > > > At this point I'd say let's go for it. > > > > Thanks, Jeff. I'll commit it to trunk if there are no further > objections some time next week. > I've just committed it as r277752. Personally I'd like to install it on GCC 8 and 9 branches as well. Any thoughts on that? Cheers, Oleg
On 11/3/19 5:12 AM, Oleg Endo wrote: > On Fri, 2019-10-11 at 23:27 +0900, Oleg Endo wrote: >> On Thu, 2019-10-03 at 19:34 -0600, Jeff Law wrote: >>> >>> So probably the most interesting target for this test is v850-elf >>> as >>> it's got a reasonably well functioning simulator, hard and soft FP >>> targets, little endian, and I'm familiar with its current set of >>> failures. >>> >>> I can confirm that your patch makes no difference in the test >>> results >>> (which includes execution results). >>> >>> In fact, there haven't been any problems on any target in my tester >>> that >>> I can tie back to this change. >>> >>> At this point I'd say let's go for it. >>> >> >> Thanks, Jeff. I'll commit it to trunk if there are no further >> objections some time next week. >> > > I've just committed it as r277752. > > Personally I'd like to install it on GCC 8 and 9 branches as well. > Any thoughts on that? Obviously we tend to be a bit more conservative the older the branch. But this is also a correctness issue. I'd suggest a week or two on the trunk, then go ahead with backporting. jeff
Index: libgcc/config/arc/t-arc =================================================================== --- libgcc/config/arc/t-arc (revision 251045) +++ libgcc/config/arc/t-arc (working copy) @@ -46,7 +46,6 @@ dp-bit.c: $(srcdir)/fp-bit.c echo '#ifndef __big_endian__' > dp-bit.c - echo '#define FLOAT_BIT_ORDER_MISMATCH' >> dp-bit.c echo '#endif' >> dp-bit.c echo '#include "fp-bit.h"' >> dp-bit.c echo '#include "config/arc/dp-hack.h"' >> dp-bit.c @@ -55,7 +54,6 @@ fp-bit.c: $(srcdir)/fp-bit.c echo '#define FLOAT' > fp-bit.c echo '#ifndef __big_endian__' >> fp-bit.c - echo '#define FLOAT_BIT_ORDER_MISMATCH' >> fp-bit.c echo '#endif' >> fp-bit.c echo '#include "config/arc/fp-hack.h"' >> fp-bit.c cat $(srcdir)/fp-bit.c >> fp-bit.c Index: libgcc/fp-bit.c =================================================================== --- libgcc/fp-bit.c (revision 251045) +++ libgcc/fp-bit.c (working copy) @@ -316,12 +316,7 @@ /* We previously used bitfields to store the number, but this doesn't handle little/big endian systems conveniently, so use shifts and masks */ -#ifdef FLOAT_BIT_ORDER_MISMATCH - dst.bits.fraction = fraction; - dst.bits.exp = exp; - dst.bits.sign = sign; -#else -# if defined TFLOAT && defined HALFFRACBITS +#if defined TFLOAT && defined HALFFRACBITS { halffractype high, low, unity; int lowsign, lowexp; @@ -394,11 +389,10 @@ } dst.value_raw = ((fractype) high << HALFSHIFT) | low; } -# else +#else dst.value_raw = fraction & ((((fractype)1) << FRACBITS) - (fractype)1); dst.value_raw |= ((fractype) (exp & ((1 << EXPBITS) - 1))) << FRACBITS; dst.value_raw |= ((fractype) (sign & 1)) << (FRACBITS | EXPBITS); -# endif #endif #if defined(FLOAT_WORD_ORDER_MISMATCH) && !defined(FLOAT) @@ -450,12 +444,7 @@ src = &swapped; #endif -#ifdef FLOAT_BIT_ORDER_MISMATCH - fraction = src->bits.fraction; - exp = src->bits.exp; - sign = src->bits.sign; -#else -# if defined TFLOAT && defined HALFFRACBITS +#if defined TFLOAT && defined HALFFRACBITS { halffractype high, low; @@ -498,11 +487,10 @@ } } } -# else +#else fraction = src->value_raw & ((((fractype)1) << FRACBITS) - 1); exp = ((int)(src->value_raw >> FRACBITS)) & ((1 << EXPBITS) - 1); sign = ((int)(src->value_raw >> (FRACBITS + EXPBITS))) & 1; -# endif #endif dst->sign = sign; Index: libgcc/fp-bit.h =================================================================== --- libgcc/fp-bit.h (revision 251045) +++ libgcc/fp-bit.h (working copy) @@ -128,10 +128,6 @@ #define NO_DI_MODE #endif -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ -#define FLOAT_BIT_ORDER_MISMATCH -#endif - #if __BYTE_ORDER__ != __FLOAT_WORD_ORDER__ #define FLOAT_WORD_ORDER_MISMATCH #endif @@ -354,16 +350,6 @@ # endif #endif -#ifdef FLOAT_BIT_ORDER_MISMATCH - struct - { - fractype fraction:FRACBITS __attribute__ ((packed)); - unsigned int exp:EXPBITS __attribute__ ((packed)); - unsigned int sign:1 __attribute__ ((packed)); - } - bits; -#endif - #ifdef _DEBUG_BITFLOAT struct {
Hi, I've been dragging this patch along with me for a while. At the moment, I don't have the resources to fully test it as requested by Ian in the PR discussion. So I would like to ask for general comments on this one and hope that folks with bigger automated test setups can run the patch through their machinery for little endian targets. Summary of the story: I've noticed this issue on the RX on GCC 6, but it seems it's been there forever. On RX, fp-bit is used for software floating point emulation. The RX target also uses "MS bit-field" layout by default. This means that code like struct { fractype fraction:FRACBITS __attribute__ ((packed)); unsigned int exp:EXPBITS __attribute__ ((packed)); unsigned int sign:1 __attribute__ ((packed)); } bits; will result in sizeof (bits) != 8 For some reason, this bit-field style declaration is used only for FLOAT_BIT_ORDER_MISMATCH, which generally seems to be set for little endian targets. In other cases (i.e. big endian) open coded bit field extraction and packing is used on the base integer type, like fraction = src->value_raw & ((((fractype)1) << FRACBITS) - 1); exp = ((int)(src->value_raw >> FRACBITS)) & ((1 << EXPBITS) - 1); sign = ((int)(src->value_raw >> (FRACBITS + EXPBITS))) & 1; This works of course regardless of the bit-field packing layout of the target. Joseph suggested to pack the struct bit, which would fix the issue. https://gcc.gnu.org/ml/gcc-bugs/2017-08/msg01651.html However, I would like to propose to remove the special case of FLOAT_BIT_ORDER_MISMATCH altogether as in the attached patch. Any comments? Cheers, Oleg libgcc/ChangeLog PR libgcc/77804 * fp-bit.h: Remove FLOAT_BIT_ORDER_MISMATCH. * fp-bit.c (pack_d, unpack_d): Remove special cases for FLOAT_BIT_ORDER_MISMATCH. * config/arc/t-arc: Remove FLOAT_BIT_ORDER_MISMATCH.