[PATCH 18/19] target/ppc: Clear fpstatus flags on VSX_CMP

Víctor Colombo posted 19 patches 3 years, 5 months ago
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>
There is a newer version of this series
[PATCH 18/19] target/ppc: Clear fpstatus flags on VSX_CMP
Posted by Víctor Colombo 3 years, 5 months ago
Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
---
 target/ppc/fpu_helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 5f7f52ab5b..fd3a966371 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2639,6 +2639,8 @@ uint32_t helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                     \
     int all_true = 1;                                                     \
     int all_false = 1;                                                    \
                                                                           \
+    helper_reset_fpstatus(env);                                           \
+                                                                          \
     for (i = 0; i < nels; i++) {                                          \
         if (unlikely(tp##_is_any_nan(xa->fld) ||                          \
                      tp##_is_any_nan(xb->fld))) {                         \
-- 
2.25.1


Re: [PATCH 18/19] target/ppc: Clear fpstatus flags on VSX_CMP
Posted by Daniel Henrique Barboza 3 years, 5 months ago

On 9/1/22 10:17, Víctor Colombo wrote:
> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
> ---

What I mentioned in patch 10 also applies to all patches from 11 to 18
it seems. All changes made in patches 09-18 are based on the explanation
gave in patch 08.

The problem with this is that it'll be annoying if/when something goes
wrong. Let's say that the change made in patch 15 caused a side-effect.
Bisect will point it to patch 15, which doesn't have an explanation of
why you made the change, and then one will need to trace it back to the
mailing list to understand it. It's not a given that one will look at
all the recent changes and understand that the logic used in patch 08
are also being used in the subsequent patches.

I don't mind if you just copy/paste the commit message from patch 08 and
just change the instruction name being fixed. What's important is to
provide some context for each individual change.


Thanks,


Daniel


>   target/ppc/fpu_helper.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 5f7f52ab5b..fd3a966371 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -2639,6 +2639,8 @@ uint32_t helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                     \
>       int all_true = 1;                                                     \
>       int all_false = 1;                                                    \
>                                                                             \
> +    helper_reset_fpstatus(env);                                           \
> +                                                                          \
>       for (i = 0; i < nels; i++) {                                          \
>           if (unlikely(tp##_is_any_nan(xa->fld) ||                          \
>                        tp##_is_any_nan(xb->fld))) {                         \

Re: [PATCH 18/19] target/ppc: Clear fpstatus flags on VSX_CMP
Posted by Víctor Colombo 3 years, 5 months ago
On 05/09/2022 15:41, Daniel Henrique Barboza wrote:
> On 9/1/22 10:17, Víctor Colombo wrote:
>> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
>> ---
> 
> What I mentioned in patch 10 also applies to all patches from 11 to 18
> it seems. All changes made in patches 09-18 are based on the explanation
> gave in patch 08.
> 
> The problem with this is that it'll be annoying if/when something goes
> wrong. Let's say that the change made in patch 15 caused a side-effect.
> Bisect will point it to patch 15, which doesn't have an explanation of
> why you made the change, and then one will need to trace it back to the
> mailing list to understand it. It's not a given that one will look at
> all the recent changes and understand that the logic used in patch 08
> are also being used in the subsequent patches.
> 
> I don't mind if you just copy/paste the commit message from patch 08 and
> just change the instruction name being fixed. What's important is to
> provide some context for each individual change.
> 
> 
> Thanks,
> 
> 
> Daniel
> 

Hello Daniel. Thank you very much for the reviews.

I'll take your recommendation and make the necessary changes.

Best regards,

-- 
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>