diff mbox

target-sh4: get rid of CPU_{Float,Double}U

Message ID 1302462785-8672-1-git-send-email-aurelien@aurel32.net
State New
Headers show

Commit Message

Aurelien Jarno April 10, 2011, 7:13 p.m. UTC 
SH4 is always using softfloat, so it's possible to have helpers directly
taking float32 or float64 value. This allow to get rid of conversions
through CPU_{Float,Double}U.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Nathan Froyd <froydnj@codesourcery.com>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-sh4/helper.h    |   48 ++++++------
 target-sh4/op_helper.c |  202 ++++++++++++++++--------------------------------
 2 files changed, 92 insertions(+), 158 deletions(-)

Comments

Nathan Froyd April 11, 2011, 2:55 p.m. UTC | #1 
On Sun, Apr 10, 2011 at 09:13:05PM +0200, Aurelien Jarno wrote:
> SH4 is always using softfloat, so it's possible to have helpers directly
> taking float32 or float64 value. This allow to get rid of conversions
> through CPU_{Float,Double}U.

Eh, I think this punning on i32/f32 and i64/f64 values is not healthy.
But Peter's already said that the floats-as-structs bit of softfloat is
broken, so maybe it's not worth trying to ensure floats-as-structs works
(or even making it the default, to discourage people from bit-twiddling
directly).

-Nathan
Peter Maydell April 11, 2011, 3:09 p.m. UTC | #2 
On 11 April 2011 15:55, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Sun, Apr 10, 2011 at 09:13:05PM +0200, Aurelien Jarno wrote:
>> SH4 is always using softfloat, so it's possible to have helpers directly
>> taking float32 or float64 value. This allow to get rid of conversions
>> through CPU_{Float,Double}U.
>
> Eh, I think this punning on i32/f32 and i64/f64 values is not healthy.
> But Peter's already said that the floats-as-structs bit of softfloat is
> broken, so maybe it's not worth trying to ensure floats-as-structs works
> (or even making it the default, to discourage people from bit-twiddling
> directly).

I guess I should clarify that about the floats-as-structs thing.

(1) It does compile cleanly for the ARM target. Some other targets
don't compile because they're (buggily) not using the boxing/unboxing
macros when they do bit-twiddling of floats; that should be fixed.
(2) I think most of the value is in whether it compiles OK or not,
rather than trying to actually run with it as a config (which I
agree with Nathan is likely to go wrong if you have a host which
doesn't pass 32/64 bit structs in registers). The compile test catches
cases where the C code is doing bit-twiddling on float32s.
(3) If we did say you shouldn't be passing 'float32' etc into helper
functions, this would make the def-helper.h support for 'f32' and 'f64'
a bit pointless because they could never be used
(4) I think you should be able to write a helper function for an
add as just
 float32 HELPER(my_float_add)(float32 a, float32 b) {
     return float32_add(a, b, status);
 }
and having to add boxing/unboxing macros to this reduces clarity
for no real gain. Using the macros should be a sign you're doing
something wrong, not that you're doing it right :-)

-- PMM
Richard Henderson April 11, 2011, 3:19 p.m. UTC | #3 
On 04/11/2011 08:09 AM, Peter Maydell wrote:
> (4) I think you should be able to write a helper function for an
> add as just
>  float32 HELPER(my_float_add)(float32 a, float32 b) {
>      return float32_add(a, b, status);
>  }

While this is a laudable goal, this will fail for hosts that pass
all structures by reference.  This is true of, e.g. PPC32.


r~
Peter Maydell April 11, 2011, 3:30 p.m. UTC | #4 
On 11 April 2011 16:19, Richard Henderson <rth@twiddle.net> wrote:
> On 04/11/2011 08:09 AM, Peter Maydell wrote:
>> (4) I think you should be able to write a helper function for an
>> add as just
>>  float32 HELPER(my_float_add)(float32 a, float32 b) {
>>      return float32_add(a, b, status);
>>  }
>
> While this is a laudable goal, this will fail for hosts that pass
> all structures by reference.  This is true of, e.g. PPC32.

...but only if float32 is a struct, which is where we came in.
In the sane default configuration float32 is just a uint32_t
in disguise.

In other words, my point is that I'd prefer to give up[*] being
able to run with float32-is-a-struct rather than give up having
clean and straightforward helper functions.

[*] actually I suspect we've never actually had this capability
so we're not really giving anything up except philosophically...

-- PMM
Aurelien Jarno April 11, 2011, 3:31 p.m. UTC | #5 
On Mon, Apr 11, 2011 at 04:09:53PM +0100, Peter Maydell wrote:
> On 11 April 2011 15:55, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > On Sun, Apr 10, 2011 at 09:13:05PM +0200, Aurelien Jarno wrote:
> >> SH4 is always using softfloat, so it's possible to have helpers directly
> >> taking float32 or float64 value. This allow to get rid of conversions
> >> through CPU_{Float,Double}U.
> >
> > Eh, I think this punning on i32/f32 and i64/f64 values is not healthy.
> > But Peter's already said that the floats-as-structs bit of softfloat is
> > broken, so maybe it's not worth trying to ensure floats-as-structs works
> > (or even making it the default, to discourage people from bit-twiddling
> > directly).
> 
> I guess I should clarify that about the floats-as-structs thing.
> 

I think I should ask some more details about this floats-as-structs
history. It has been added when a lot of targets were still using
softfloat-native and it was still possible to think about being able to
switch from one to another (at least having this goal). The idea was to
avoid doing float computation on the wrong value. For example this code
does work with softfloat-native, but will produce wrong results with
softfloat:

  float64 fadd(float64 a, float64 b)
  {
      return a + b;
  }

The problem with this kind of code is that it compiles both with
softfloat and softfloat-native, but gives wrong results on softfloat.
By adding this floats-as-structs, it was possible to catch such errors
during compile time.

However now that most targets (except i386) default to softfloat, we
don't really need to forbid bit twiddling in target code.
Richard Henderson April 11, 2011, 3:32 p.m. UTC | #6 
On 04/11/2011 08:30 AM, Peter Maydell wrote:
> On 11 April 2011 16:19, Richard Henderson <rth@twiddle.net> wrote:
>> On 04/11/2011 08:09 AM, Peter Maydell wrote:
>>> (4) I think you should be able to write a helper function for an
>>> add as just
>>>  float32 HELPER(my_float_add)(float32 a, float32 b) {
>>>      return float32_add(a, b, status);
>>>  }
>>
>> While this is a laudable goal, this will fail for hosts that pass
>> all structures by reference.  This is true of, e.g. PPC32.
> 
> ...but only if float32 is a struct, which is where we came in.
> In the sane default configuration float32 is just a uint32_t
> in disguise.

Well, that's all right then.  So long as we restrict ourselves
to passing around (typedefed) integers and pointers only, we'll
be ok.


r~
diff mbox

Patch

diff --git a/target-sh4/helper.h b/target-sh4/helper.h
index 2e52768..95e3c7c 100644
--- a/target-sh4/helper.h
+++ b/target-sh4/helper.h
@@ -23,31 +23,31 @@  DEF_HELPER_2(macw, void, i32, i32)
 
 DEF_HELPER_1(ld_fpscr, void, i32)
 
-DEF_HELPER_1(fabs_FT, i32, i32)
-DEF_HELPER_1(fabs_DT, i64, i64)
-DEF_HELPER_2(fadd_FT, i32, i32, i32)
-DEF_HELPER_2(fadd_DT, i64, i64, i64)
-DEF_HELPER_1(fcnvsd_FT_DT, i64, i32)
-DEF_HELPER_1(fcnvds_DT_FT, i32, i64)
+DEF_HELPER_1(fabs_FT, f32, f32)
+DEF_HELPER_1(fabs_DT, f64, f64)
+DEF_HELPER_2(fadd_FT, f32, f32, f32)
+DEF_HELPER_2(fadd_DT, f64, f64, f64)
+DEF_HELPER_1(fcnvsd_FT_DT, f64, f32)
+DEF_HELPER_1(fcnvds_DT_FT, f32, f64)
 
-DEF_HELPER_2(fcmp_eq_FT, void, i32, i32)
-DEF_HELPER_2(fcmp_eq_DT, void, i64, i64)
-DEF_HELPER_2(fcmp_gt_FT, void, i32, i32)
-DEF_HELPER_2(fcmp_gt_DT, void, i64, i64)
-DEF_HELPER_2(fdiv_FT, i32, i32, i32)
-DEF_HELPER_2(fdiv_DT, i64, i64, i64)
-DEF_HELPER_1(float_FT, i32, i32)
-DEF_HELPER_1(float_DT, i64, i32)
-DEF_HELPER_3(fmac_FT, i32, i32, i32, i32)
-DEF_HELPER_2(fmul_FT, i32, i32, i32)
-DEF_HELPER_2(fmul_DT, i64, i64, i64)
-DEF_HELPER_1(fneg_T, i32, i32)
-DEF_HELPER_2(fsub_FT, i32, i32, i32)
-DEF_HELPER_2(fsub_DT, i64, i64, i64)
-DEF_HELPER_1(fsqrt_FT, i32, i32)
-DEF_HELPER_1(fsqrt_DT, i64, i64)
-DEF_HELPER_1(ftrc_FT, i32, i32)
-DEF_HELPER_1(ftrc_DT, i32, i64)
+DEF_HELPER_2(fcmp_eq_FT, void, f32, f32)
+DEF_HELPER_2(fcmp_eq_DT, void, f64, f64)
+DEF_HELPER_2(fcmp_gt_FT, void, f32, f32)
+DEF_HELPER_2(fcmp_gt_DT, void, f64, f64)
+DEF_HELPER_2(fdiv_FT, f32, f32, f32)
+DEF_HELPER_2(fdiv_DT, f64, f64, f64)
+DEF_HELPER_1(float_FT, f32, i32)
+DEF_HELPER_1(float_DT, f64, i32)
+DEF_HELPER_3(fmac_FT, f32, f32, f32, f32)
+DEF_HELPER_2(fmul_FT, f32, f32, f32)
+DEF_HELPER_2(fmul_DT, f64, f64, f64)
+DEF_HELPER_1(fneg_T, f32, f32)
+DEF_HELPER_2(fsub_FT, f32, f32, f32)
+DEF_HELPER_2(fsub_DT, f64, f64, f64)
+DEF_HELPER_1(fsqrt_FT, f32, f32)
+DEF_HELPER_1(fsqrt_DT, f64, f64)
+DEF_HELPER_1(ftrc_FT, i32, f32)
+DEF_HELPER_1(ftrc_DT, i32, f64)
 DEF_HELPER_2(fipr, void, i32, i32)
 DEF_HELPER_1(ftrv, void, i32)
 
diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c
index b8f4ca2..c127860 100644
--- a/target-sh4/op_helper.c
+++ b/target-sh4/op_helper.c
@@ -487,53 +487,38 @@  static void update_fpscr(void *retaddr)
     }
 }
 
-uint32_t helper_fabs_FT(uint32_t t0)
+float32 helper_fabs_FT(float32 t0)
 {
-    CPU_FloatU f;
-    f.l = t0;
-    f.f = float32_abs(f.f);
-    return f.l;
+    return float32_abs(t0);
 }
 
-uint64_t helper_fabs_DT(uint64_t t0)
+float64 helper_fabs_DT(float64 t0)
 {
-    CPU_DoubleU d;
-    d.ll = t0;
-    d.d = float64_abs(d.d);
-    return d.ll;
+    return float64_abs(t0);
 }
 
-uint32_t helper_fadd_FT(uint32_t t0, uint32_t t1)
+float32 helper_fadd_FT(float32 t0, float32 t1)
 {
-    CPU_FloatU f0, f1;
-    f0.l = t0;
-    f1.l = t1;
     set_float_exception_flags(0, &env->fp_status);
-    f0.f = float32_add(f0.f, f1.f, &env->fp_status);
+    t0 = float32_add(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-    return f0.l;
+    return t0;
 }
 
-uint64_t helper_fadd_DT(uint64_t t0, uint64_t t1)
+float64 helper_fadd_DT(float64 t0, float64 t1)
 {
-    CPU_DoubleU d0, d1;
-    d0.ll = t0;
-    d1.ll = t1;
     set_float_exception_flags(0, &env->fp_status);
-    d0.d = float64_add(d0.d, d1.d, &env->fp_status);
+    t0 = float64_add(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-    return d0.ll;
+    return t0;
 }
 
-void helper_fcmp_eq_FT(uint32_t t0, uint32_t t1)
+void helper_fcmp_eq_FT(float32 t0, float32 t1)
 {
-    CPU_FloatU f0, f1;
     int relation;
-    f0.l = t0;
-    f1.l = t1;
 
     set_float_exception_flags(0, &env->fp_status);
-    relation = float32_compare(f0.f, f1.f, &env->fp_status);
+    relation = float32_compare(t0, t1, &env->fp_status);
     if (unlikely(relation == float_relation_unordered)) {
         update_fpscr(GETPC());
     } else if (relation == float_relation_equal) {
@@ -543,15 +528,12 @@  void helper_fcmp_eq_FT(uint32_t t0, uint32_t t1)
     }
 }
 
-void helper_fcmp_eq_DT(uint64_t t0, uint64_t t1)
+void helper_fcmp_eq_DT(float64 t0, float64 t1)
 {
-    CPU_DoubleU d0, d1;
     int relation;
-    d0.ll = t0;
-    d1.ll = t1;
 
     set_float_exception_flags(0, &env->fp_status);
-    relation = float64_compare(d0.d, d1.d, &env->fp_status);
+    relation = float64_compare(t0, t1, &env->fp_status);
     if (unlikely(relation == float_relation_unordered)) {
         update_fpscr(GETPC());
     } else if (relation == float_relation_equal) {
@@ -561,15 +543,12 @@  void helper_fcmp_eq_DT(uint64_t t0, uint64_t t1)
     }
 }
 
-void helper_fcmp_gt_FT(uint32_t t0, uint32_t t1)
+void helper_fcmp_gt_FT(float32 t0, float32 t1)
 {
-    CPU_FloatU f0, f1;
     int relation;
-    f0.l = t0;
-    f1.l = t1;
 
     set_float_exception_flags(0, &env->fp_status);
-    relation = float32_compare(f0.f, f1.f, &env->fp_status);
+    relation = float32_compare(t0, t1, &env->fp_status);
     if (unlikely(relation == float_relation_unordered)) {
         update_fpscr(GETPC());
     } else if (relation == float_relation_greater) {
@@ -579,15 +558,12 @@  void helper_fcmp_gt_FT(uint32_t t0, uint32_t t1)
     }
 }
 
-void helper_fcmp_gt_DT(uint64_t t0, uint64_t t1)
+void helper_fcmp_gt_DT(float64 t0, float64 t1)
 {
-    CPU_DoubleU d0, d1;
     int relation;
-    d0.ll = t0;
-    d1.ll = t1;
 
     set_float_exception_flags(0, &env->fp_status);
-    relation = float64_compare(d0.d, d1.d, &env->fp_status);
+    relation = float64_compare(t0, t1, &env->fp_status);
     if (unlikely(relation == float_relation_unordered)) {
         update_fpscr(GETPC());
     } else if (relation == float_relation_greater) {
@@ -597,176 +573,134 @@  void helper_fcmp_gt_DT(uint64_t t0, uint64_t t1)
     }
 }
 
-uint64_t helper_fcnvsd_FT_DT(uint32_t t0)
+float64 helper_fcnvsd_FT_DT(float32 t0)
 {
-    CPU_DoubleU d;
-    CPU_FloatU f;
-    f.l = t0;
+    float64 ret;
     set_float_exception_flags(0, &env->fp_status);
-    d.d = float32_to_float64(f.f, &env->fp_status);
+    ret = float32_to_float64(t0, &env->fp_status);
     update_fpscr(GETPC());
-    return d.ll;
+    return ret;
 }
 
-uint32_t helper_fcnvds_DT_FT(uint64_t t0)
+float32 helper_fcnvds_DT_FT(float64 t0)
 {
-    CPU_DoubleU d;
-    CPU_FloatU f;
-    d.ll = t0;
+    float32 ret;
     set_float_exception_flags(0, &env->fp_status);
-    f.f = float64_to_float32(d.d, &env->fp_status);
+    ret = float64_to_float32(t0, &env->fp_status);
     update_fpscr(GETPC());
-    return f.l;
+    return ret;
 }
 
-uint32_t helper_fdiv_FT(uint32_t t0, uint32_t t1)
+float32 helper_fdiv_FT(float32 t0, float32 t1)
 {
-    CPU_FloatU f0, f1;
-    f0.l = t0;
-    f1.l = t1;
     set_float_exception_flags(0, &env->fp_status);
-    f0.f = float32_div(f0.f, f1.f, &env->fp_status);
+    t0 = float32_div(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-    return f0.l;
+    return t0;
 }
 
-uint64_t helper_fdiv_DT(uint64_t t0, uint64_t t1)
+float64 helper_fdiv_DT(float64 t0, float64 t1)
 {
-    CPU_DoubleU d0, d1;
-    d0.ll = t0;
-    d1.ll = t1;
     set_float_exception_flags(0, &env->fp_status);
-    d0.d = float64_div(d0.d, d1.d, &env->fp_status);
+    t0 = float64_div(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-    return d0.ll;
+    return t0;
 }
 
-uint32_t helper_float_FT(uint32_t t0)
+float32 helper_float_FT(uint32_t t0)
 {
-    CPU_FloatU f;
-
+    float32 ret;
     set_float_exception_flags(0, &env->fp_status);
-    f.f = int32_to_float32(t0, &env->fp_status);
+    ret = int32_to_float32(t0, &env->fp_status);
     update_fpscr(GETPC());
-
-    return f.l;
+    return ret;
 }
 
-uint64_t helper_float_DT(uint32_t t0)
+float64 helper_float_DT(uint32_t t0)
 {
-    CPU_DoubleU d;
+    float64 ret;
     set_float_exception_flags(0, &env->fp_status);
-    d.d = int32_to_float64(t0, &env->fp_status);
+    ret = int32_to_float64(t0, &env->fp_status);
     update_fpscr(GETPC());
-    return d.ll;
+    return ret;
 }
 
-uint32_t helper_fmac_FT(uint32_t t0, uint32_t t1, uint32_t t2)
+float32 helper_fmac_FT(float32 t0, float32 t1, float32 t2)
 {
-    CPU_FloatU f0, f1, f2;
-    f0.l = t0;
-    f1.l = t1;
-    f2.l = t2;
     set_float_exception_flags(0, &env->fp_status);
-    f0.f = float32_mul(f0.f, f1.f, &env->fp_status);
-    f0.f = float32_add(f0.f, f2.f, &env->fp_status);
+    t0 = float32_mul(t0, t1, &env->fp_status);
+    t0 = float32_add(t0, t2, &env->fp_status);
     update_fpscr(GETPC());
-
-    return f0.l;
+    return t0;
 }
 
-uint32_t helper_fmul_FT(uint32_t t0, uint32_t t1)
+float32 helper_fmul_FT(float32 t0, float32 t1)
 {
-    CPU_FloatU f0, f1;
-    f0.l = t0;
-    f1.l = t1;
     set_float_exception_flags(0, &env->fp_status);
-    f0.f = float32_mul(f0.f, f1.f, &env->fp_status);
+    t0 = float32_mul(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-    return f0.l;
+    return t0;
 }
 
-uint64_t helper_fmul_DT(uint64_t t0, uint64_t t1)
+float64 helper_fmul_DT(float64 t0, float64 t1)
 {
-    CPU_DoubleU d0, d1;
-    d0.ll = t0;
-    d1.ll = t1;
     set_float_exception_flags(0, &env->fp_status);
-    d0.d = float64_mul(d0.d, d1.d, &env->fp_status);
+    t0 = float64_mul(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-
-    return d0.ll;
+    return t0;
 }
 
-uint32_t helper_fneg_T(uint32_t t0)
+float32 helper_fneg_T(float32 t0)
 {
-    CPU_FloatU f;
-    f.l = t0;
-    f.f = float32_chs(f.f);
-    return f.l;
+    return float32_chs(t0);
 }
 
-uint32_t helper_fsqrt_FT(uint32_t t0)
+float32 helper_fsqrt_FT(float32 t0)
 {
-    CPU_FloatU f;
-    f.l = t0;
     set_float_exception_flags(0, &env->fp_status);
-    f.f = float32_sqrt(f.f, &env->fp_status);
+    t0 = float32_sqrt(t0, &env->fp_status);
     update_fpscr(GETPC());
-    return f.l;
+    return t0;
 }
 
-uint64_t helper_fsqrt_DT(uint64_t t0)
+float64 helper_fsqrt_DT(float64 t0)
 {
-    CPU_DoubleU d;
-    d.ll = t0;
     set_float_exception_flags(0, &env->fp_status);
-    d.d = float64_sqrt(d.d, &env->fp_status);
+    t0 = float64_sqrt(t0, &env->fp_status);
     update_fpscr(GETPC());
-    return d.ll;
+    return t0;
 }
 
-uint32_t helper_fsub_FT(uint32_t t0, uint32_t t1)
+float32 helper_fsub_FT(float32 t0, float32 t1)
 {
-    CPU_FloatU f0, f1;
-    f0.l = t0;
-    f1.l = t1;
     set_float_exception_flags(0, &env->fp_status);
-    f0.f = float32_sub(f0.f, f1.f, &env->fp_status);
+    t0 = float32_sub(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-    return f0.l;
+    return t0;
 }
 
-uint64_t helper_fsub_DT(uint64_t t0, uint64_t t1)
+float64 helper_fsub_DT(float64 t0, float64 t1)
 {
-    CPU_DoubleU d0, d1;
-
-    d0.ll = t0;
-    d1.ll = t1;
     set_float_exception_flags(0, &env->fp_status);
-    d0.d = float64_sub(d0.d, d1.d, &env->fp_status);
+    t0 = float64_sub(t0, t1, &env->fp_status);
     update_fpscr(GETPC());
-    return d0.ll;
+    return t0;
 }
 
-uint32_t helper_ftrc_FT(uint32_t t0)
+uint32_t helper_ftrc_FT(float32 t0)
 {
-    CPU_FloatU f;
     uint32_t ret;
-    f.l = t0;
     set_float_exception_flags(0, &env->fp_status);
-    ret = float32_to_int32_round_to_zero(f.f, &env->fp_status);
+    ret = float32_to_int32_round_to_zero(t0, &env->fp_status);
     update_fpscr(GETPC());
     return ret;
 }
 
-uint32_t helper_ftrc_DT(uint64_t t0)
+uint32_t helper_ftrc_DT(float64 t0)
 {
-    CPU_DoubleU d;
     uint32_t ret;
-    d.ll = t0;
     set_float_exception_flags(0, &env->fp_status);
-    ret = float64_to_int32_round_to_zero(d.d, &env->fp_status);
+    ret = float64_to_int32_round_to_zero(t0, &env->fp_status);
     update_fpscr(GETPC());
     return ret;
 }