1. Patchew
  2. QEMU
  3. View series

[PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64

Li-Hang Lin posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251226064225.791454-1-lihang.lin@gmail.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>
target/arm/tcg/vfp_helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Li-Hang Lin 1 month, 1 week ago 
Fix an error in rsqrte_f64() where the sign bit was being
placed incorrectly. Specifically, ensure f64_sign is deposited
into bit 63.

Additionally, update the comments regarding the exponent (exp) bit
width to reflect the correct double-precision specifications.

Signed-off-by: Li-Hang Lin <lihang.lin@gmail.com>
---
 target/arm/tcg/vfp_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/tcg/vfp_helper.c b/target/arm/tcg/vfp_helper.c
index e156e3774a..60188b2c7e 100644
--- a/target/arm/tcg/vfp_helper.c
+++ b/target/arm/tcg/vfp_helper.c
@@ -1078,8 +1078,8 @@ float64 HELPER(rsqrte_f64)(float64 input, float_status *s)
 
     f64_frac = recip_sqrt_estimate(&f64_exp, 3068, f64_frac, false);
 
-    /* result = sign : result_exp<4:0> : estimate<7:0> : Zeros(44) */
-    val = deposit64(0, 61, 1, f64_sign);
+    /* result = sign : result_exp<10:0> : estimate<7:0> : Zeros(44) */
+    val = deposit64(0, 63, 1, f64_sign);
     val = deposit64(val, 52, 11, f64_exp);
     val = deposit64(val, 44, 8, extract64(f64_frac, 52 - 8, 8));
     return make_float64(val);
-- 
2.52.0.351.gbe84eed79e-goog
Re: [PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Peter Maydell 1 month, 1 week ago 
On Fri, 26 Dec 2025 at 06:43, Li-Hang Lin <lihang.lin@gmail.com> wrote:
>
> Fix an error in rsqrte_f64() where the sign bit was being
> placed incorrectly. Specifically, ensure f64_sign is deposited
> into bit 63.
>
> Additionally, update the comments regarding the exponent (exp) bit
> width to reflect the correct double-precision specifications.

This seems like it would produce incorrect results -- do you
have an example of an instruction plus input data values that p
produces a different output value to the hardware?

thanks
-- PMM
Re: [PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Lin Li-Hang 1 month, 1 week ago 
Hi Peter,

Thank you for your reply.

I initially identified this error while reviewing the code and was curious
why it hadn't caused any bugs.
After further testing, it appears the original code behaved correctly by
coincidence.

The ASL code in the ARM ARM for FRSQRTE states:

```
elsif sign == '1' then
         result = FPDefaultNaN(fpcr, N);
         if fpexc then FPProcessException(FPExc_InvalidOp, fpcr);
```

As it turns out, the sign bit must be zero by the time it reaches the final
deposition code, which explains why the incorrect bit placement did not
surface as a functional bug.


On Mon, Dec 29, 2025 at 1:15 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Fri, 26 Dec 2025 at 06:43, Li-Hang Lin <lihang.lin@gmail.com> wrote:
> >
> > Fix an error in rsqrte_f64() where the sign bit was being
> > placed incorrectly. Specifically, ensure f64_sign is deposited
> > into bit 63.
> >
> > Additionally, update the comments regarding the exponent (exp) bit
> > width to reflect the correct double-precision specifications.
>
> This seems like it would produce incorrect results -- do you
> have an example of an instruction plus input data values that p
> produces a different output value to the hardware?
>
> thanks
> -- PMM
>
Re: [PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Peter Maydell 3 weeks, 4 days ago 
On Mon, 29 Dec 2025 at 04:19, Lin Li-Hang <lihang.lin@gmail.com> wrote:
>
> Hi Peter,
>
> Thank you for your reply.
>
> I initially identified this error while reviewing the code and was curious why it hadn't caused any bugs.
> After further testing, it appears the original code behaved correctly by coincidence.
>
> The ASL code in the ARM ARM for FRSQRTE states:
>
> ```
> elsif sign == '1' then
>          result = FPDefaultNaN(fpcr, N);
>          if fpexc then FPProcessException(FPExc_InvalidOp, fpcr);
> ```
>
> As it turns out, the sign bit must be zero by the time it reaches the final deposition code, which explains why the incorrect bit placement did not surface as a functional bug.

Thanks for looking that up. I think that although this isn't a bug it's
definitely confusing code, so the best approach will be to make our
code match how the current Arm ARM FPRSqrtEstimate() treats the output
sign bit, which is to say we know it's 0. In the pseudocode that
looks like:
  result = '0' : result_exp<N-54:0> : estimate<7:0>:Zeros(44);
and for QEMU it should look like updating the comment so that
instead of
/* result = sign : result_exp<4:0> : estimate<7:0> : Zeros(44) */

it says
/* result = 0 : result_exp<4:0> : estimate<7:0> : Zeros(44) */
and removing the unnecessary deposit64() call of f64_sign entirely.

We should do this for all of rsqrte_f64, do_rsqrte_f32 and rsqrte_f16.

thanks
-- PMM
Re: [PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Lin Li-Hang 3 weeks, 4 days ago 
Hi Peter,

Thanks for your explanation.
Your statement and suggestion is better.

Cheers


On Mon, Jan 12, 2026 at 10:36 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 29 Dec 2025 at 04:19, Lin Li-Hang <lihang.lin@gmail.com> wrote:
> >
> > Hi Peter,
> >
> > Thank you for your reply.
> >
> > I initially identified this error while reviewing the code and was
> curious why it hadn't caused any bugs.
> > After further testing, it appears the original code behaved correctly by
> coincidence.
> >
> > The ASL code in the ARM ARM for FRSQRTE states:
> >
> > ```
> > elsif sign == '1' then
> >          result = FPDefaultNaN(fpcr, N);
> >          if fpexc then FPProcessException(FPExc_InvalidOp, fpcr);
> > ```
> >
> > As it turns out, the sign bit must be zero by the time it reaches the
> final deposition code, which explains why the incorrect bit placement did
> not surface as a functional bug.
>
> Thanks for looking that up. I think that although this isn't a bug it's
> definitely confusing code, so the best approach will be to make our
> code match how the current Arm ARM FPRSqrtEstimate() treats the output
> sign bit, which is to say we know it's 0. In the pseudocode that
> looks like:
>   result = '0' : result_exp<N-54:0> : estimate<7:0>:Zeros(44);
> and for QEMU it should look like updating the comment so that
> instead of
> /* result = sign : result_exp<4:0> : estimate<7:0> : Zeros(44) */
>
> it says
> /* result = 0 : result_exp<4:0> : estimate<7:0> : Zeros(44) */
> and removing the unnecessary deposit64() call of f64_sign entirely.
>
> We should do this for all of rsqrte_f64, do_rsqrte_f32 and rsqrte_f16.
>
> thanks
> -- PMM
>
Re: [PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Lin Li-Hang 1 month ago 
Hi Peter,

Is my patch good for you?

Cheers


On Mon, Dec 29, 2025 at 12:19 PM Lin Li-Hang <lihang.lin@gmail.com> wrote:

> Hi Peter,
>
> Thank you for your reply.
>
> I initially identified this error while reviewing the code and was curious
> why it hadn't caused any bugs.
> After further testing, it appears the original code behaved correctly by
> coincidence.
>
> The ASL code in the ARM ARM for FRSQRTE states:
>
> ```
> elsif sign == '1' then
>          result = FPDefaultNaN(fpcr, N);
>          if fpexc then FPProcessException(FPExc_InvalidOp, fpcr);
> ```
>
> As it turns out, the sign bit must be zero by the time it reaches the
> final deposition code, which explains why the incorrect bit placement did
> not surface as a functional bug.
>
>
> On Mon, Dec 29, 2025 at 1:15 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On Fri, 26 Dec 2025 at 06:43, Li-Hang Lin <lihang.lin@gmail.com> wrote:
>> >
>> > Fix an error in rsqrte_f64() where the sign bit was being
>> > placed incorrectly. Specifically, ensure f64_sign is deposited
>> > into bit 63.
>> >
>> > Additionally, update the comments regarding the exponent (exp) bit
>> > width to reflect the correct double-precision specifications.
>>
>> This seems like it would produce incorrect results -- do you
>> have an example of an instruction plus input data values that p
>> produces a different output value to the hardware?
>>
>> thanks
>> -- PMM
>>
>
Re: [PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Philippe Mathieu-Daudé 1 month, 1 week ago 
On 26/12/25 07:42, Li-Hang Lin wrote:
> Fix an error in rsqrte_f64() where the sign bit was being
> placed incorrectly. Specifically, ensure f64_sign is deposited
> into bit 63.
> 
> Additionally, update the comments regarding the exponent (exp) bit
> width to reflect the correct double-precision specifications.
> 

Fixes: d719cbc7641 ("arm/helper.c: re-factor rsqrte and add rsqrte_f16")

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Li-Hang Lin <lihang.lin@gmail.com>
> ---
>   target/arm/tcg/vfp_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/tcg/vfp_helper.c b/target/arm/tcg/vfp_helper.c
> index e156e3774a..60188b2c7e 100644
> --- a/target/arm/tcg/vfp_helper.c
> +++ b/target/arm/tcg/vfp_helper.c
> @@ -1078,8 +1078,8 @@ float64 HELPER(rsqrte_f64)(float64 input, float_status *s)
>   
>       f64_frac = recip_sqrt_estimate(&f64_exp, 3068, f64_frac, false);
>   
> -    /* result = sign : result_exp<4:0> : estimate<7:0> : Zeros(44) */
> -    val = deposit64(0, 61, 1, f64_sign);
> +    /* result = sign : result_exp<10:0> : estimate<7:0> : Zeros(44) */
> +    val = deposit64(0, 63, 1, f64_sign);
>       val = deposit64(val, 52, 11, f64_exp);
>       val = deposit64(val, 44, 8, extract64(f64_frac, 52 - 8, 8));
>       return make_float64(val);
Re: [PATCH] target/arm/tcg/vfp_helper: Fix incorrect bit field deposition in rsqrte_f64
Posted by Michael Tokarev 1 month, 1 week ago 
On 12/26/25 18:19, Philippe Mathieu-Daudé wrote:
> On 26/12/25 07:42, Li-Hang Lin wrote:
>> Fix an error in rsqrte_f64() where the sign bit was being
>> placed incorrectly. Specifically, ensure f64_sign is deposited
>> into bit 63.
>>
>> Additionally, update the comments regarding the exponent (exp) bit
>> width to reflect the correct double-precision specifications.
>>
> 
> Fixes: d719cbc7641 ("arm/helper.c: re-factor rsqrte and add rsqrte_f16")

That's.. 2.12, 2018 - not too far ago :)

Cc: qemu-stable@nongnu.org.

/mjt

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.