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
