diff mbox series

libgccjit: Fix float playback for cross-compilation

Message ID ce5b809b30de16c037120c35859e5180903aa949.camel@zoho.com
State Accepted
Headers
Series libgccjit: Fix float playback for cross-compilation |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Antoni Boucher Jan. 11, 2024, 11:42 p.m. UTC 
  Hi.
This patch fixes the bug 113343.
I'm wondering if there's a better solution than using mpfr.
The only other solution I found is real_from_string, but that seems
overkill to convert the number to a string.
I could not find a better way to create a real value from a host
double.
If there's no solution, do we lose some precision by using mpfr?
Running Rust's core library tests, there was a difference of one
decimal, so I'm wondering if there's some lost precision, or if it's
just because those tests don't work on m68k which was my test target.
Also, I'm not sure how to write a test this fix. Any ideas?
Thanks for the review.

Comments

David Malcolm Jan. 24, 2024, 6:10 p.m. UTC | #1 
On Thu, 2024-01-11 at 18:42 -0500, Antoni Boucher wrote:
> Hi.
> This patch fixes the bug 113343.
> I'm wondering if there's a better solution than using mpfr.
> The only other solution I found is real_from_string, but that seems
> overkill to convert the number to a string.
> I could not find a better way to create a real value from a host
> double.

I took a look, and I don't see a better way; it seems weird to go
through a string stage.  Ideally there would be a
real_from_host_double, but I don't see one.

Is there a cross-platform way to directly access the representation of
a host double?

> If there's no solution, do we lose some precision by using mpfr?
> Running Rust's core library tests, there was a difference of one
> decimal, so I'm wondering if there's some lost precision, or if it's
> just because those tests don't work on m68k which was my test target.

Sorry, can you clarify what you mean by "a difference of one decimal"
above?

> Also, I'm not sure how to write a test this fix. Any ideas?

I think we don't need cross-compilation-specific tests, we should just
use and/or extend the existing coverage for
gcc_jit_context_new_rvalue_from_double e.g. in test-constants.c and
test-types.c

We probably should have test coverage for "awkward" values; we already
have coverage for DBL_MIN and DBL_MAX, but we don't yet have test
coverage for:
* quiet/signaling NaN
* +ve/-ve inf
* -ve zero

Thanks
Dave
Antoni Boucher Jan. 25, 2024, 9:04 p.m. UTC | #2 
Thanks for the review!

On Wed, 2024-01-24 at 13:10 -0500, David Malcolm wrote:
> On Thu, 2024-01-11 at 18:42 -0500, Antoni Boucher wrote:
> > Hi.
> > This patch fixes the bug 113343.
> > I'm wondering if there's a better solution than using mpfr.
> > The only other solution I found is real_from_string, but that seems
> > overkill to convert the number to a string.
> > I could not find a better way to create a real value from a host
> > double.
> 
> I took a look, and I don't see a better way; it seems weird to go
> through a string stage.  Ideally there would be a
> real_from_host_double, but I don't see one.
> 
> Is there a cross-platform way to directly access the representation
> of
> a host double?

I have no idea.

> 
> > If there's no solution, do we lose some precision by using mpfr?
> > Running Rust's core library tests, there was a difference of one
> > decimal, so I'm wondering if there's some lost precision, or if
> > it's
> > just because those tests don't work on m68k which was my test
> > target.
> 
> Sorry, can you clarify what you mean by "a difference of one decimal"
> above?

Let's say the Rust core tests expected the value "1.23456789", it
instead got the value "1.2345678" (e.g. without the last decimal).
Not sure if this is expected.
Everything works fine for x86-64; this just happened for m68k which is
not well supported for now in Rust, so that might just be that the test
doesn't work on this platform.

> 
> > Also, I'm not sure how to write a test this fix. Any ideas?
> 
> I think we don't need cross-compilation-specific tests, we should
> just
> use and/or extend the existing coverage for
> gcc_jit_context_new_rvalue_from_double e.g. in test-constants.c and
> test-types.c
> 
> We probably should have test coverage for "awkward" values; we
> already
> have coverage for DBL_MIN and DBL_MAX, but we don't yet have test
> coverage for:
> * quiet/signaling NaN
> * +ve/-ve inf
> * -ve zero

Is this something you would want for this patch?

> 
> Thanks
> Dave
>
Antoni Boucher Feb. 17, 2024, 4:56 p.m. UTC | #3 
David: Ping.

On Thu, 2024-01-25 at 16:04 -0500, Antoni Boucher wrote:
> Thanks for the review!
> 
> On Wed, 2024-01-24 at 13:10 -0500, David Malcolm wrote:
> > On Thu, 2024-01-11 at 18:42 -0500, Antoni Boucher wrote:
> > > Hi.
> > > This patch fixes the bug 113343.
> > > I'm wondering if there's a better solution than using mpfr.
> > > The only other solution I found is real_from_string, but that
> > > seems
> > > overkill to convert the number to a string.
> > > I could not find a better way to create a real value from a host
> > > double.
> > 
> > I took a look, and I don't see a better way; it seems weird to go
> > through a string stage.  Ideally there would be a
> > real_from_host_double, but I don't see one.
> > 
> > Is there a cross-platform way to directly access the representation
> > of
> > a host double?
> 
> I have no idea.
> 
> > 
> > > If there's no solution, do we lose some precision by using mpfr?
> > > Running Rust's core library tests, there was a difference of one
> > > decimal, so I'm wondering if there's some lost precision, or if
> > > it's
> > > just because those tests don't work on m68k which was my test
> > > target.
> > 
> > Sorry, can you clarify what you mean by "a difference of one
> > decimal"
> > above?
> 
> Let's say the Rust core tests expected the value "1.23456789", it
> instead got the value "1.2345678" (e.g. without the last decimal).
> Not sure if this is expected.
> Everything works fine for x86-64; this just happened for m68k which
> is
> not well supported for now in Rust, so that might just be that the
> test
> doesn't work on this platform.
> 
> > 
> > > Also, I'm not sure how to write a test this fix. Any ideas?
> > 
> > I think we don't need cross-compilation-specific tests, we should
> > just
> > use and/or extend the existing coverage for
> > gcc_jit_context_new_rvalue_from_double e.g. in test-constants.c and
> > test-types.c
> > 
> > We probably should have test coverage for "awkward" values; we
> > already
> > have coverage for DBL_MIN and DBL_MAX, but we don't yet have test
> > coverage for:
> > * quiet/signaling NaN
> > * +ve/-ve inf
> > * -ve zero
> 
> Is this something you would want for this patch?
> 
> > 
> > Thanks
> > Dave
> > 
>
diff mbox series

Patch

From 8ddfd4abbe6e46efc256030c2d010f035cd9ecf0 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Sat, 21 Oct 2023 11:20:46 -0400
Subject: [PATCH] libgccjit: Fix float playback for cross-compilation

gcc/jit/ChangeLog:
	PR jit/113343
	* jit-playback.cc (new_rvalue_from_const): Fix to have the
	correct value when cross-compiling.
---
 gcc/jit/jit-playback.cc | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index dddd537f3b1..9cb27ee4ef3 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -41,6 +41,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "gcc.h"
 #include "diagnostic.h"
 #include "stmt.h"
+#include "realmpfr.h"
 
 #include "jit-playback.h"
 #include "jit-result.h"
@@ -932,22 +933,16 @@  new_rvalue_from_const <double> (type *type,
   // FIXME: type-checking, or coercion?
   tree inner_type = type->as_tree ();
 
+  mpfr_t mpf_value;
+
+  mpfr_init2 (mpf_value, 64);
+  mpfr_set_d (mpf_value, value, MPFR_RNDN);
+
   /* We have a "double", we want a REAL_VALUE_TYPE.
 
-     real.cc:real_from_target appears to require the representation to be
-     split into 32-bit values, and then sent as an pair of host long
-     ints.  */
+     realmpfr.cc:real_from_mpfr.  */
   REAL_VALUE_TYPE real_value;
-  union
-  {
-    double as_double;
-    uint32_t as_uint32s[2];
-  } u;
-  u.as_double = value;
-  long int as_long_ints[2];
-  as_long_ints[0] = u.as_uint32s[0];
-  as_long_ints[1] = u.as_uint32s[1];
-  real_from_target (&real_value, as_long_ints, DFmode);
+  real_from_mpfr (&real_value, mpf_value, inner_type, MPFR_RNDN);
   tree inner = build_real (inner_type, real_value);
   return new rvalue (this, inner);
 }
-- 
2.43.0