| Message ID | 1317652087-27288-1-git-send-email-christophe.lyon@st.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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); }
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(-)