Re: [Qemu-devel] [2.3 V2 PATCH 2/6] target-ppc: Fix Floating Point Move Instructions That Set CR1

[Alexander Graf]

On 20.11.14 15:32, Tom Musta wrote:
> On 11/20/2014 8:14 AM, Alexander Graf wrote:
>>
>>
>> On 12.11.14 22:46, Tom Musta wrote:
>>> The Floating Point Move instructions (fmr., fabs., fnabs., fneg.,
>>> and fcpsgn.) incorrectly copy FPSCR[FPCC] instead of [FX,FEX,VX,OX].
>>> Furthermore, the current code does this via a call to gen_compute_fprf,
>>> which is awkward since these instructions do not actually set FPRF.
>>>
>>> Change the code to use the gen_set_cr1_from_fpscr utility.
>>>
>>> Signed-off-by: Tom Musta <address@hidden>
>>> ---
>>>  target-ppc/translate.c |   50 
>>> ++++++++++++++++++++++++++++-------------------
>>>  1 files changed, 30 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>> index 910ce56..2d79e39 100644
>>> --- a/target-ppc/translate.c
>>> +++ b/target-ppc/translate.c
>>> @@ -2077,6 +2077,21 @@ static void gen_srd(DisasContext *ctx)
>>>  }
>>>  #endif
>>>  
>>> +#if defined(TARGET_PPC64)
>>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
>>> +{
>>> +    TCGv_i32 tmp = tcg_temp_new_i32();
>>> +    tcg_gen_trunc_tl_i32(tmp, cpu_fpscr);
>>> +    tcg_gen_shri_i32(cpu_crf[1], tmp, 28);
>>> +    tcg_temp_free_i32(tmp);
>>> +}
>>> +#else
>>> +static void gen_set_cr1_from_fpscr(DisasContext *ctx)
>>> +{
>>> +        tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
>>> +}
>>> +#endif
>>> +
>>>  /***                       Floating-Point arithmetic                       
>>> ***/
>>>  #define _GEN_FLOAT_ACB(name, op, op1, op2, isfloat, set_fprf, type)        
>>>    \
>>>  static void gen_f##name(DisasContext *ctx)                                 
>>>    \
>>> @@ -2370,7 +2385,9 @@ static void gen_fabs(DisasContext *ctx)
>>>      }
>>>      tcg_gen_andi_i64(cpu_fpr[rD(ctx->opcode)], cpu_fpr[rB(ctx->opcode)],
>>>                       ~(1ULL << 63));
>>> -    gen_compute_fprf(cpu_fpr[rD(ctx->opcode)], 0, Rc(ctx->opcode) != 0);
>>> +    if (unlikely(Rc(ctx->opcode))) {
>>> +        gen_set_cr1_from_fpscr(ctx);
>>> +    }
>>
>> I don't quite understand this. We set cr1 based on fpscr, but we don't
>> recalculate the respective fpscr bits?
>>
>> Wouldn't we get outdated comparison data?
>>
>>
>> Alex
>>
> 
> Nope.
> 
> The floating point move instructions don't actually even alter the FPSCR.  
> From the ISA (see the last sentence):
> 
> 4.6.5 Floating-Point Move Instructions
> These instructions copy data from one floating-point
> register to another, altering the sign bit (bit 0) as
> described below for fneg, fabs, fnabs, and fcpsgn.
> These instructions treat NaNs just like any other kind of
> value (e.g., the sign bit of a NaN may be altered by
> fneg, fabs, fnabs, and fcpsgn). These instructions do
> not alter the FPSCR.

Thanks, applied with the following squashed in:

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 7aef089..35c3a16 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -2088,7 +2088,7 @@ static void gen_set_cr1_from_fpscr(DisasContext *ctx)
 #else
 static void gen_set_cr1_from_fpscr(DisasContext *ctx)
 {
-        tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
+    tcg_gen_shri_tl(cpu_crf[1], cpu_fpscr, 28);
 }
 #endif


Alex