Re: An first try to improve PPC float simulation, not even compiled. Just ask question.

[Richard Henderson <richard.henderson@linaro.org>]

On 5/4/20 11:30 AM, BALATON Zoltan wrote:
> On Mon, 4 May 2020, Richard Henderson wrote:
>> On 5/3/20 5:41 PM, 罗勇刚(Yonggang Luo) wrote:
>>> On Mon, May 4, 2020 at 7:40 AM BALATON Zoltan <balaton@eik.bme.hu
>>> <mailto:balaton@eik.bme.hu>> wrote:
>>>
>>>     Hello,
>>>
>>>     On Mon, 4 May 2020, 罗勇刚(Yonggang Luo) wrote:
>>>    > Hello Richard, Can you have a look at the following patch, and was that
>>> are
>>>    > the right direction?
>>>
>>>     Formatting of the patch is broken by your mailer, try sending it with
>>>     something that does not change it otherwise it's a bit hard to read.
>>>
>>>     Richard suggested to add an assert to check the fp_status is correctly
>>>     cleared in place of helper_reset_fpstatus first for debugging so you could
>>>     change the helper accordingly before deleting it and run a few tests to
>>>     verify it still works. You'll need get some tests and benchmarks working
>>>     to be able to verify your changes that's why I've said that would be step
>>>     0. If you checked that it still produces the same results and the assert
>>>     does not trigger then you can remove the helper.
>>>
>>> That's what I need help,
>>> 1. How to write a assert to replace helper_reset_fpstatus .
>>>   just directly assert? or something else
>>
>> You can't place the assert where helper_reset_fpstatus was.  You need to place
>> it in each of the helpers, like helper_fadd, that previously has a call to
>> helper_reset_fpstatus preceeding it.
> 
> Why? If we want to verify that clearing fp_status after flags are processed is
> equivalent to clearing flags before fp ops then verifying that the fp_status is
> already cleared when the current helper_reset_fpstatus is called should be
> enough to check that nothing has set the flags in between so the current reset
> helper would be no op. Therefore I thought you could put the assert there for
> checking this. This assert is for debugging and checking the change only and
> not meant to be left there otherwise we lose all the performance gain so it's
> easier to put in the current helper before removing it for this than in every
> fp op helper. What am I missing?

I'm not sure what you are suggesting.

If you are suggesting

 void helper_reset_fpstatus(CPUPPCState *env)
 {
-    set_float_exception_flags(0, &env->fp_status);
+    assert(get_float_exception_flags(&env->fp_status) == 0);
 }

then, sure, that works.  But we also want to remove that call, so in order to
retain the check for debugging, we need to move the assert into the other helpers.


r~