[patch,avr] Fix PR target/99184: Wrong cast from double to 16-bit and 32-bit ints.

[patch,avr] Fix PR target/99184: Wrong cast from double to 16-bit and 32-bit ints.

[Georg Johann Lay]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

Hello,

this patch fixed PR target/99184 which incorrectly rounded during 64-bit 
(long) double to 16-bit and 32-bit integers.

The patch just removes the respective roundings from 
libf7-asm.sx::to_integer and ::to_unsigned.  Luckily, LibF7 does nowhere 
use respective functions internally, the only user is in libf7.c::f7_exp

which reads

   f7_round (qq, qq);
   int16_t q = f7_get_s16 (qq);

so that f7_get_s16() operates on an already rounded value, and therefore 
this code works unaltered with or without rounding in to_integer.

The patch applies to directory

./libgcc/config/avr/libf7/

and is the same for all GCC versions v10+.

Please someone with write permissions commit it to trunk and backport to 
v12, v11, and v10 as it is a wrong-code issue.

The patch will fit without problems (except for ChangeLog) because there 
is no traffic on that folder.


Thanks, Johann


libgcc/config/avr/libf7/
	PR target/99184
	Remove rounding from double to [u]int16 and [u]int32 casts.

	* libf7-asm.sx (to_integer, to_unsigned): Don't round 16-bit
	and 32-bit integers.


[-- Attachment #2: pr99184.diff --]
[-- Type: text/x-patch, Size: 2035 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 7e06f52..3ec0082 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+
+	PR target/99184
+	Remove rounding from double to [u]int16 and [u]int32 casts.
+
+	* libf7-asm.sx (to_integer, to_unsigned): Don't round 16-bit
+	and 32-bit integers.
+
 2022-04-21  Release Manager
 
 	* GCC 11.3.0 released.
diff --git a/libf7-asm.sx b/libf7-asm.sx
index 7629e23..9d701f2 100644
--- a/libf7-asm.sx
+++ b/libf7-asm.sx
@@ -601,9 +601,6 @@ DEFUN to_integer
     tst     C6
     brmi    .Lsaturate.T    ;   > INTxx_MAX  =>  saturate
 
-    rcall   .Lround
-    brmi    .Lsaturate.T    ;   > INTxx_MAX  =>  saturate
-
     brtc 9f                 ;   >= 0         =>  return
     sbrc    Mask,   5
     .global __negdi2
@@ -658,30 +655,6 @@ DEFUN to_integer
     .global __clr_8
     XJMP    __clr_8
 
-.Lround:
-    ;; C6.7 is known to be 0 here.
-    ;; Return N = 1 iff we have to saturate.
-    cpi     Mask,   0xf
-    breq .Lround16
-    cpi     Mask,   0x1f
-    breq .Lround32
-
-    ;; For now, no rounding in the 64-bit case.  This rounding
-    ;; would have to be integrated into the right-shift.
-    cln
-    ret
-
-.Lround32:
-    rol     C2
-    adc     C3,     ZERO
-    adc     C4,     ZERO
-    rjmp 2f
-
-.Lround16:
-    rol     C4
-2:  adc     C5,     ZERO
-    adc     C6,     ZERO
-    ret
 ENDF to_integer
 #endif /* F7MOD_to_integer_ */
 
@@ -725,29 +698,6 @@ DEFUN to_unsigned
     clr     CA
     F7call  lshrdi3
     POP     r16
-
-    ;; Rounding
-    ;; ??? C6.7 is known to be 0 here.
-    cpi     Mask,   0xf
-    breq .Lround16
-    cpi     Mask,   0x1f
-    breq .Lround32
-
-    ;; For now, no rounding in the 64-bit case.  This rounding
-    ;; would have to be integrated into the right-shift.
-    ret
-
-.Lround32:
-    rol     C2
-    adc     C3,     ZERO
-    adc     C4,     ZERO
-    rjmp 2f
-
-.Lround16:
-    rol     C4
-2:  adc     C5,     ZERO
-    adc     C6,     ZERO
-    brcs    .Lset_0xffff    ; Rounding overflow  =>  saturate
     ret
 
 .Lset_0xffff:

Re: [patch, avr] Fix PR target/99184: Wrong cast from double to 16-bit and 32-bit ints.

[Richard Biener]

On Sun, Sep 18, 2022 at 7:40 PM Georg Johann Lay <avr@gjlay.de> wrote:
>
> Hello,
>
> this patch fixed PR target/99184 which incorrectly rounded during 64-bit
> (long) double to 16-bit and 32-bit integers.
>
> The patch just removes the respective roundings from
> libf7-asm.sx::to_integer and ::to_unsigned.  Luckily, LibF7 does nowhere
> use respective functions internally, the only user is in libf7.c::f7_exp
>
> which reads
>
>    f7_round (qq, qq);
>    int16_t q = f7_get_s16 (qq);
>
> so that f7_get_s16() operates on an already rounded value, and therefore
> this code works unaltered with or without rounding in to_integer.
>
> The patch applies to directory
>
> ./libgcc/config/avr/libf7/
>
> and is the same for all GCC versions v10+.
>
> Please someone with write permissions commit it to trunk and backport to
> v12, v11, and v10 as it is a wrong-code issue.
>
> The patch will fit without problems (except for ChangeLog) because there
> is no traffic on that folder.

Thanks, I've pushed the change.  Please in future try to send patches
that can be applied with git am, thus use git format-patch

Richard.

>
> Thanks, Johann
>
>
> libgcc/config/avr/libf7/
>         PR target/99184
>         Remove rounding from double to [u]int16 and [u]int32 casts.
>
>         * libf7-asm.sx (to_integer, to_unsigned): Don't round 16-bit
>         and 32-bit integers.
>

Re: [patch, avr] Fix PR target/99184: Wrong cast from double to 16-bit and 32-bit ints.

[Georg Johann Lay]

Am 19.09.22 um 09:51 schrieb Richard Biener:
> On Sun, Sep 18, 2022 at 7:40 PM Georg Johann Lay <avr@gjlay.de> wrote:
>>
>> Hello,
>>
>> this patch fixed PR target/99184 which incorrectly rounded during 64-bit
>> (long) double to 16-bit and 32-bit integers.
>>
>> The patch just removes the respective roundings from
>> libf7-asm.sx::to_integer and ::to_unsigned.  Luckily, LibF7 does nowhere
>> use respective functions internally, the only user is in libf7.c::f7_exp
>>
>> which reads
>>
>>     f7_round (qq, qq);
>>     int16_t q = f7_get_s16 (qq);
>>
>> so that f7_get_s16() operates on an already rounded value, and therefore
>> this code works unaltered with or without rounding in to_integer.
>>
>> The patch applies to directory
>>
>> ./libgcc/config/avr/libf7/
>>
>> and is the same for all GCC versions v10+.
>>
>> Please someone with write permissions commit it to trunk and backport to
>> v12, v11, and v10 as it is a wrong-code issue.
>>
>> The patch will fit without problems (except for ChangeLog) because there
>> is no traffic on that folder.
> 
> Thanks, I've pushed the change.  Please in future try to send patches
> that can be applied with git am, thus use git format-patch
> 
> Richard.

Thanks you so much.  The patch I generated with "git diff > file.diff", 
so that is not correct? The only change is that I defined extra hunks 
for asm so that one can see the function like in

  @@ -601,9 +601,6 @@ DEFUN to_integer

So git is not prepared to such hunks? Would you point me to some 
documentation on how to do it properly?

Thanks,

Johann

Re: [patch, avr] Fix PR target/99184: Wrong cast from double to 16-bit and 32-bit ints.

[Richard Biener]

On Mon, Sep 19, 2022 at 10:57 AM Georg Johann Lay <avr@gjlay.de> wrote:
>
>
>
> Am 19.09.22 um 09:51 schrieb Richard Biener:
> > On Sun, Sep 18, 2022 at 7:40 PM Georg Johann Lay <avr@gjlay.de> wrote:
> >>
> >> Hello,
> >>
> >> this patch fixed PR target/99184 which incorrectly rounded during 64-bit
> >> (long) double to 16-bit and 32-bit integers.
> >>
> >> The patch just removes the respective roundings from
> >> libf7-asm.sx::to_integer and ::to_unsigned.  Luckily, LibF7 does nowhere
> >> use respective functions internally, the only user is in libf7.c::f7_exp
> >>
> >> which reads
> >>
> >>     f7_round (qq, qq);
> >>     int16_t q = f7_get_s16 (qq);
> >>
> >> so that f7_get_s16() operates on an already rounded value, and therefore
> >> this code works unaltered with or without rounding in to_integer.
> >>
> >> The patch applies to directory
> >>
> >> ./libgcc/config/avr/libf7/
> >>
> >> and is the same for all GCC versions v10+.
> >>
> >> Please someone with write permissions commit it to trunk and backport to
> >> v12, v11, and v10 as it is a wrong-code issue.
> >>
> >> The patch will fit without problems (except for ChangeLog) because there
> >> is no traffic on that folder.
> >
> > Thanks, I've pushed the change.  Please in future try to send patches
> > that can be applied with git am, thus use git format-patch
> >
> > Richard.
>
> Thanks you so much.  The patch I generated with "git diff > file.diff",
> so that is not correct?

Yes, it's correct, but see below.

> The only change is that I defined extra hunks
> for asm so that one can see the function like in
>
>   @@ -601,9 +601,6 @@ DEFUN to_integer
>
> So git is not prepared to such hunks? Would you point me to some
> documentation on how to do it properly?

The more useful process is that after changing the code you
do a git commit to your tree and then

git format-patch --stdout HEAD^

to get a git patch.  On that I can do 'git am' and that will produce
exactly the commit you produced, including the patch description
and ChangeLog parts.  I had to add that manually.

Indeed https://gcc.gnu.org/contribute.html doesn't mention this,
we should probably improve that.  Jonathan, maybe the above
is not the optimal way so can you think of something to add
to the 'Submitting Patches' section to document this?

Thanks,
Richard.

>
> Thanks,
>
> Johann

Re: [patch, avr] Fix PR target/99184: Wrong cast from double to 16-bit and 32-bit ints.

[Jonathan Wakely]

On Mon, 19 Sept 2022 at 10:06, Richard Biener wrote:
>
> On Mon, Sep 19, 2022 at 10:57 AM Georg Johann Lay <avr@gjlay.de> wrote:
> >
> >
> >
> > Am 19.09.22 um 09:51 schrieb Richard Biener:
> > > On Sun, Sep 18, 2022 at 7:40 PM Georg Johann Lay <avr@gjlay.de> wrote:
> > >>
> > >> Hello,
> > >>
> > >> this patch fixed PR target/99184 which incorrectly rounded during 64-bit
> > >> (long) double to 16-bit and 32-bit integers.
> > >>
> > >> The patch just removes the respective roundings from
> > >> libf7-asm.sx::to_integer and ::to_unsigned.  Luckily, LibF7 does nowhere
> > >> use respective functions internally, the only user is in libf7.c::f7_exp
> > >>
> > >> which reads
> > >>
> > >>     f7_round (qq, qq);
> > >>     int16_t q = f7_get_s16 (qq);
> > >>
> > >> so that f7_get_s16() operates on an already rounded value, and therefore
> > >> this code works unaltered with or without rounding in to_integer.
> > >>
> > >> The patch applies to directory
> > >>
> > >> ./libgcc/config/avr/libf7/
> > >>
> > >> and is the same for all GCC versions v10+.
> > >>
> > >> Please someone with write permissions commit it to trunk and backport to
> > >> v12, v11, and v10 as it is a wrong-code issue.
> > >>
> > >> The patch will fit without problems (except for ChangeLog) because there
> > >> is no traffic on that folder.
> > >
> > > Thanks, I've pushed the change.  Please in future try to send patches
> > > that can be applied with git am, thus use git format-patch
> > >
> > > Richard.
> >
> > Thanks you so much.  The patch I generated with "git diff > file.diff",
> > so that is not correct?
>
> Yes, it's correct, but see below.

I would say it's just wrong.

> > The only change is that I defined extra hunks
> > for asm so that one can see the function like in
> >
> >   @@ -601,9 +601,6 @@ DEFUN to_integer
> >
> > So git is not prepared to such hunks? Would you point me to some
> > documentation on how to do it properly?
>
> The more useful process is that after changing the code you
> do a git commit to your tree and then
>
> git format-patch --stdout HEAD^
>
> to get a git patch.  On that I can do 'git am' and that will produce
> exactly the commit you produced, including the patch description
> and ChangeLog parts.  I had to add that manually.
>
> Indeed https://gcc.gnu.org/contribute.html doesn't mention this,
> we should probably improve that.  Jonathan, maybe the above
> is not the optimal way so can you think of something to add
> to the 'Submitting Patches' section to document this?

I'll try to come up with something.

I agree that format-patch (or just 'git show') is the proper way to
generate a patch for submission to the mailing lists. Just doing 'git
diff' only works if you haven't committed or staged any part of your
work, and frankly if you haven't even got to the point of committing
it locally, it's just a rough draft and probably not ready to submit
to the mailing list.

The current docs do not really represent anything close to best
practice when using Git for version control. But last time I tried to
improve those docs I gave up because people wanted perfection instead
of "good enough" (compared to "quite bad" as we have now).