Re: [RFC PATCH v2] target/ppc: Enable hardfloat for PPC

[BALATON Zoltan]

On Thu, 20 Feb 2020, Richard Henderson wrote:
> On 2/18/20 9:10 AM, BALATON Zoltan wrote:
> >  void helper_reset_fpstatus(CPUPPCState *env)
> >  {
> > -    set_float_exception_flags(0, &env->fp_status);
> > +    set_float_exception_flags(env->default_fp_excpt_flags, &env->fp_status);
> >  }
>
> What I don't like is the forced setting of inexact.  I don't mind leaving it
> set if it is already set, which corresponds to the normal accumulation of
> exceptions.

In a previous reply I've explained this (apart from just trying to get it 
work the simplest way for testing first):

On Wed, 19 Feb 2020, BALATON Zoltan wrote:
> Some of the problems with inexact may be fixed by not always forcing the 
> flag on but just not clearing it. As I undersood other targets do that 
> so it starts with softfloat but the first time the inexact flag is set 
> it will start using hardfloat as long as the guest does not clear this 
> flag. Probably this is done to automatically detect code that needs the 
> flag and assume it's not used when it's not touched. Since PPC also has 
> an inexact flag just for previous FP op (the FI bit) apart from the 
> usual cumulative flag, the client could read that instead of clearing 
> the cumulative flag so we can't detect guest usage this way, therefore 
> we might as well break inexact completely to always use hardfloat and 
> need to manually enable it for guests that we know need it. I'm not sure 
> however if forcing the inexact flag would lead to unwanted FP exceptions 
> as well so this may also need to be made conditional on the 
> enabled/disabled status of inexact FP exceptions. Does anyone have more 
> info on this?

So I know this should be improved but not sure how. I would not worry 
about setting the bit from the start due to the above but exceptions are 
likely wrong this way as you say.

> In addition, if the inexact exception is unmasked, I would expect a signal to
> be delivered only when an inexact exception happens.  Whereas this patch would
> deliver a signal for every fp operation.
>
> It should be just as easy to do
>
>    flags = get_float_exception_flags(status);
>    flags &= env->save_fp_exception_flags;
>    set_float_exception_flags(flags, status);

I'm not sure I get this. Can you please explain a bit more? Where would 
save_flags be set and where would the above masking with it be done? 
Currently we reset flags before every FP op as far as I understand and 
detecting of exceptions rely on this and test fp_status bits. Probably we 
should not reset the flags and change this to keep one fp_status 
accumulating flags and only reset if softfloat is forced but that would 
need rewriting exception handling that I'm not sure yet how to do. Any 
idea? Maybe if we could just unbreak inexact exceptions when enabled with 
current patch then this rewrite of exception handling could be done later.

> > +    DEFINE_PROP_BOOL("hardfloat", PowerPCCPU, hardfloat, true),
>
> I would also prefer a different name here -- perhaps x-no-fp-fi.

What's wrong with hardfloat? That's how the code refers to this so if 
anyone searches what it does would turn up some meaningful results. Your 
proposed property name is very cryptic and nobody would be able to guess 
what is it for. If you insist not to use hardfloat maybe we could invert 
it to softfloat or try x-use-fpu maybe. I specificaly don't want to tie it 
to the FI but as there's at least another non-sticky bit in FPSCR (don't 
remember which now) that might also need to be broken for best hardfloat 
results.

Regards,
BALATON Zoltan