Re: [Qemu-devel] [RFC PATCH 1/6] target/ppc: introduce get_fpr() and set_fpr() helpers for FP register access

[Mark Cave-Ayland]

On 10/12/2018 18:43, Richard Henderson wrote:

> On 12/7/18 2:56 AM, Mark Cave-Ayland wrote:
>> -    gen_helper_f##op(cpu_fpr[rD(ctx->opcode)], cpu_env,                     
>>   \
>> -                     cpu_fpr[rA(ctx->opcode)],                              
>>   \
>> -                     cpu_fpr[rC(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);   
>>   \
>> +    get_fpr(t0, rA(ctx->opcode));                                           
>>   \
>> +    get_fpr(t1, rC(ctx->opcode));                                           
>>   \
>> +    get_fpr(t2, rB(ctx->opcode));                                           
>>   \
>> +    gen_helper_f##op(t3, cpu_env, t0, t1, t2);                              
>>   \
>> +    set_fpr(rD(ctx->opcode), t3);                                           
>>   \
>>      if (isfloat) {                                                          
>>   \
>> -        gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                  
>>   \
>> -                        cpu_fpr[rD(ctx->opcode)]);                          
>>   \
>> +        get_fpr(t0, rD(ctx->opcode));                                       
>>   \
>> +        gen_helper_frsp(t3, cpu_env, t0);                                   
>>   \
>> +        set_fpr(rD(ctx->opcode), t3);                                       
>>   \
>>      }                                                                       
>>   \
> 
> This is an accurate conversion, but the writeback to the rD register need not
> happen until after helper_frsp.  Just move it below the isfloat block.

Okay - I'll fix this up in the next iteration.

> I do see that helper_frsp can raise an exception for invalid_op for SNaN.  If
> that code were actually reachable, this would have been an existing bug, in
> that we should not have written back to rD after the first operation.  
> However,
> any SNaN will already have been eliminated by the first operation (via
> squashing to QNaN or by exiting via exception).
> 
> Similarly in GEN_FLOAT_AB.

Noted.

>> +    get_fpr(t0, rB(ctx->opcode));
>> +    gen_helper_frsqrte(t1, cpu_env, t0);
>> +    set_fpr(rD(ctx->opcode), t1);
>> +    gen_helper_frsp(t1, cpu_env, t1);
>> +    gen_compute_fprf_float64(t1);
> 
> gen_frsqrtes has the set_fpr in the wrong place.  Likewise gen_fsqrts.

Ooops. I remember having a think a bit harder about these two functions, so 
I'll go
and take another look.


ATB,

Mark.