[Qemu-devel] ppc: TCG and FP exceptions, is it right ?

[Qemu-devel] ppc: TCG and FP exceptions, is it right ?

[Benjamin Herrenschmidt]

Hi folks !

I could use a bit of help determining if something is quite right in
TCG emulation of FP ops.

Fabien, Tristan, you submitted a patch back in 2013:

db72c9f256ae70b30c5d5985234f085df4226c55 "powerpc: correctly handle fpu
exceptions" which effectively makes any of the exceptions generated
for underflow, overflow and inexact fire right away
(helper_raise_exception_err will exit the cpu loop with a setjmp).

This makes them effectively do the same thing
as float_zero_divide_excp().

However I *think* it might not be what we want, according to the
comment in helper_float_check_status() that says

 /* Differred floating-point exception after target FPR update */

And according to the architecture definition for those exceptions,
where we indeed want the target FPR updated before we take the
interrupts.

The code as writte will take the exception before the FPR is updated,
I *think*, or am I missing something here  ?

I think the intent was to return to the translated code so the FPRons
update happen, though we ideally would need to also set some state
so the translated code itself can then check for an exception and
fire it.

However as you noticed, that doesn't work well either.

What do you think is the most appropriate implementation here?

I'm thinking it's almost worth bringing FE0/FE1 into the hflags
so that we know at translation time whether to be precise, imprecise,
or ignore FP exceptions.

Then we could do something along the lines of:
 
  - In the helpers, when checking status, set an env flag if an
exception should occur.

  - In all the translate call sites, if FE0/1 is non-0 (at translate
time), generate call to check that flag and shoot the exception

  - Optionally, we could even implement some smarts to defer this to
the end of a TB in imprecise mode.

An additional note is that if FE0/FE1 are 0, we still in some case
leave an exception behind in cs->exception_index. Now, I *think*
that's ok, it will just be silently dropped at some point, but I am not
100% certain as that's a part of TCG I'm an not super familiar with
yet.

What do you guys reckon ? I am missing something here ?

Cheers,
Ben.

Re: [Qemu-devel] ppc: TCG and FP exceptions, is it right ?

[Richard Henderson]

On 07/25/2016 03:14 PM, Benjamin Herrenschmidt wrote:
> The code as writte will take the exception before the FPR is updated,
> I *think*, or am I missing something here  ?

You are correct, exceptions are raised from e.g. helper_fadd before the 
writeback to the fp register.

> What do you think is the most appropriate implementation here?

Something like the sparc implementation.

We compute the value with one helper, exception bits are kept in the fp_status 
struct.  Writeback to the register happens at the end of the call to the first 
helper.  We then immediately call a second helper, 
helper_check_ieee_exceptions, which takes the data from fp_status and produces 
the appropriate sparc status bits and raises whatever exceptions are required.

> I'm thinking it's almost worth bringing FE0/FE1 into the hflags
> so that we know at translation time whether to be precise, imprecise,
> or ignore FP exceptions.

Precise or imprecise probably doesn't matter (I haven't figured a good way to 
exploit that for Alpha; not that I'm trying anymore, since ev67, which I 
emulate the most, requires precise exceptions).

But being able to know that no exceptions can be raised may be helpful.  One 
can mark the helpers as not reading all global registers, which avoids 
unnecessary memory traffic to env.

> Then we could do something along the lines of:
>
>   - In the helpers, when checking status, set an env flag if an
> exception should occur.
>
>   - In all the translate call sites, if FE0/1 is non-0 (at translate
> time), generate call to check that flag and shoot the exception

This is another plausible implementation.  I'd have to look back at the manual 
to give more specific suggestions.

> An additional note is that if FE0/FE1 are 0, we still in some case
> leave an exception behind in cs->exception_index. Now, I *think*
> that's ok, it will just be silently dropped at some point, but I am not
> 100% certain as that's a part of TCG I'm an not super familiar with
> yet.

That should be fine.  All exits from the loop should set a new value here as 
they exit.


r~

Re: [Qemu-devel] ppc: TCG and FP exceptions, is it right ?

[Tristan Gingold]

> On 25 Jul 2016, at 11:44, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> Hi folks !
> 
> I could use a bit of help determining if something is quite right in
> TCG emulation of FP ops.
> 
> Fabien, Tristan, you submitted a patch back in 2013:
> 
> db72c9f256ae70b30c5d5985234f085df4226c55 "powerpc: correctly handle fpu
> exceptions" which effectively makes any of the exceptions generated
> for underflow, overflow and inexact fire right away
> (helper_raise_exception_err will exit the cpu loop with a setjmp).
> 
> This makes them effectively do the same thing
> as float_zero_divide_excp().
> 
> However I *think* it might not be what we want, according to the
> comment in helper_float_check_status() that says
> 
>  /* Differred floating-point exception after target FPR update */
> 
> And according to the architecture definition for those exceptions,
> where we indeed want the target FPR updated before we take the
> interrupts.
> 
> The code as writte will take the exception before the FPR is updated,
> I *think*, or am I missing something here  ?

This looks like an oversight. In our tests, we don't read the fpr.

> 
> I think the intent was to return to the translated code so the FPRons
> update happen, though we ideally would need to also set some state
> so the translated code itself can then check for an exception and
> fire it.
> 
> However as you noticed, that doesn't work well either.
> 
> What do you think is the most appropriate implementation here?
> 
> I'm thinking it's almost worth bringing FE0/FE1 into the hflags
> so that we know at translation time whether to be precise, imprecise,
> or ignore FP exceptions.
> 
> Then we could do something along the lines of:
>  
>   - In the helpers, when checking status, set an env flag if an
> exception should occur.
> 
>   - In all the translate call sites, if FE0/1 is non-0 (at translate
> time), generate call to check that flag and shoot the exception
> 
>   - Optionally, we could even implement some smarts to defer this to
> the end of a TB in imprecise mode.
> 
> An additional note is that if FE0/FE1 are 0, we still in some case
> leave an exception behind in cs->exception_index. Now, I *think*
> that's ok, it will just be silently dropped at some point, but I am not
> 100% certain as that's a part of TCG I'm an not super familiar with
> yet.
> 
> What do you guys reckon ? I am missing something here ?

I like the second helper suggestion from rth@.  That's simple.

Tristan.

Re: [Qemu-devel] ppc: TCG and FP exceptions, is it right ?

[Benjamin Herrenschmidt]

On Mon, 2016-07-25 at 19:05 +0530, Richard Henderson wrote:
> > An additional note is that if FE0/FE1 are 0, we still in some case
> > leave an exception behind in cs->exception_index. Now, I *think*
> > that's ok, it will just be silently dropped at some point, but I am not
> > 100% certain as that's a part of TCG I'm an not super familiar with
> > yet.
> 
> That should be fine.  All exits from the loop should set a new value here as 
> they exit.

Right though it *might* interact with the trace exception code (single
step), I'll have at to dig a bit.

I suspect I need to produce test cases :-)

Cheers,
Ben.

Re: [Qemu-devel] ppc: TCG and FP exceptions, is it right ?

[Benjamin Herrenschmidt]

On Mon, 2016-07-25 at 17:12 +0200, Tristan Gingold wrote:
> > 
> > The code as writte will take the exception before the FPR is
> > updated,
> > I *think*, or am I missing something here  ?
> 
> This looks like an oversight. In our tests, we don't read the fpr.

BTW. Do you happen to have some tests I could run to verify that my
changes don't break your code ?

Cheers,
Ben.