[Qemu-devel] [PATCH] rsqrte_f32: No need to copy sign bit.

[Qemu-devel] [PATCH] rsqrte_f32: No need to copy sign bit.

[Christophe Lyon]

Indeed, the result is known to be always positive.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
---
 target-arm/helper.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d3a3ba2..ff5456c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3039,8 +3039,7 @@ float32 HELPER(rsqrte_f32)(float32 a, CPUState *env)
 
     val64 = float64_val(f64);
 
-    val = ((val64 >> 63)  & 0x80000000)
-        | ((result_exp & 0xff) << 23)
+    val = ((result_exp & 0xff) << 23)
         | ((val64 >> 29)  & 0x7fffff);
     return make_float32(val);
 }
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH] rsqrte_f32: No need to copy sign bit.

[Peter Maydell]

On 3 October 2011 15:28, Christophe Lyon <christophe.lyon@st.com> wrote:
> Indeed, the result is known to be always positive.

> -    val = ((val64 >> 63)  & 0x80000000)
> -        | ((result_exp & 0xff) << 23)
> +    val = ((result_exp & 0xff) << 23)
>         | ((val64 >> 29)  & 0x7fffff);
>     return make_float32(val);

So we weren't generating incorrect results, we were just doing
slightly more work than we really needed, right? I'm curious
what prompted this patch :-)

-- PMM

Re: [Qemu-devel] [PATCH] rsqrte_f32: No need to copy sign bit.

[Christophe Lyon]

On 09.10.2011 00:57, Peter Maydell wrote:
> On 3 October 2011 15:28, Christophe Lyon<christophe.lyon@st.com>  wrote:
>> Indeed, the result is known to be always positive.
>> -    val = ((val64>>  63)&  0x80000000)
>> -        | ((result_exp&  0xff)<<  23)
>> +    val = ((result_exp&  0xff)<<  23)
>>          | ((val64>>  29)&  0x7fffff);
>>      return make_float32(val);
> So we weren't generating incorrect results, we were just doing
> slightly more work than we really needed, right? I'm curious
> what prompted this patch :-)
>
Exactly. And no way to expose a bug :-(

I was reading 2 revisions of the ARM ARM, and noticed erratas in the descriptions of FPRecipEstimate and FPRSqrtEstimate.

Sign propagation has been removed in the former, and not in the later, so I re-read both functions carefully as well as qemu's implementation and came to this conclusion :-)

I have contacted ARM support and suggested them to fix both function accordingly :-)

Christophe.

Re: [Qemu-devel] [PATCH] rsqrte_f32: No need to copy sign bit.

[Peter Maydell]

On 10 October 2011 12:26, Christophe Lyon <christophe.lyon@st.com> wrote:
> On 09.10.2011 00:57, Peter Maydell wrote:
>> So we weren't generating incorrect results, we were just doing
>> slightly more work than we really needed, right? I'm curious
>> what prompted this patch :-)
>>
> Exactly. And no way to expose a bug :-(

If we always generate the correct results for all inputs, then
by definition it's not a bug. It's just a slightly suboptimal
calculation (both here and in the ARM ARM pseudocode).

> I was reading 2 revisions of the ARM ARM, and noticed erratas in the
> descriptions of FPRecipEstimate and FPRSqrtEstimate.
>
> Sign propagation has been removed in the former, and not in the later, so I
> re-read both functions carefully as well as qemu's implementation and came
> to this conclusion :-)

For FPRecipEstimate the change in the sign handling was fixing a genuine
erratum in the pseudocode. (QEMU's code for that is correct; I recall
checking at the time that we were following the amended pseudocode
rather than the old version.) For FPSqrtEstimate it's just a clarity
of phrasing issue.

Anyway, I agree we should make this change and have tested that
we still generate correct results. So:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM