Re: [PATCH] libgccjit: Fix float playback for cross-compilation

[Antoni Boucher <bouanto@zoho.com>]

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
>