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

[Tom Musta]

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.