| Message ID | 20220901131756.26060-19-victor.colombo@eldorado.org.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multiple ppc instructions fixes | expand |
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))) { \
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,
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))) { \
Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br> --- target/ppc/fpu_helper.c | 2 ++ 1 file changed, 2 insertions(+)