Re: [Qemu-devel] [PATCH 01/14] target/ppc: remove getVSR()/putVSR() from fpu_helper.c

[Richard Henderson]

On 4/28/19 7:38 AM, Mark Cave-Ayland wrote:
>  void helper_xsaddqp(CPUPPCState *env, uint32_t opcode)
>  {
> -    ppc_vsr_t xt, xa, xb;
> +    ppc_vsr_t *xt = &env->vsr[rD(opcode) + 32];
> +    ppc_vsr_t *xa = &env->vsr[rA(opcode) + 32];
> +    ppc_vsr_t *xb = &env->vsr[rB(opcode) + 32];
>      float_status tstat;
>  
> -    getVSR(rA(opcode) + 32, &xa, env);
> -    getVSR(rB(opcode) + 32, &xb, env);
> -    getVSR(rD(opcode) + 32, &xt, env);
>      helper_reset_fpstatus(env);
>  
>      tstat = env->fp_status;
> @@ -1860,18 +1857,17 @@ void helper_xsaddqp(CPUPPCState *env, uint32_t opcode)
>      }
>  
>      set_float_exception_flags(0, &tstat);
> -    xt.f128 = float128_add(xa.f128, xb.f128, &tstat);
> +    xt->f128 = float128_add(xa->f128, xb->f128, &tstat);
>      env->fp_status.float_exception_flags |= tstat.float_exception_flags;
>  
>      if (unlikely(tstat.float_exception_flags & float_flag_invalid)) {
>          float_invalid_op_addsub(env, 1, GETPC(),
> -                                float128_classify(xa.f128) |
> -                                float128_classify(xb.f128));
> +                                float128_classify(xa->f128) |
> +                                float128_classify(xb->f128));

These values are no longer valid, because you may have written over them with
the store to xt->f128.  You need to keep the result in a local variable until
the location of the putVSR in order to keep the current semantics.

(Although the current semantics probably need to be reviewed with respect to
how the exception is signaled vs the result is stored to the register file.  I
know there are current bugs in this area with respect to regular floating-point
operations, never mind the vector floating-point ones.)

>  #define VSX_ADD_SUB(name, op, nels, tp, fld, sfprf, r2sp)                    
> \
>  void helper_##name(CPUPPCState *env, uint32_t opcode)                        
> \
>  {                                                                            
> \
> -    ppc_vsr_t xt, xa, xb;                                                    
> \
> +    ppc_vsr_t *xt = &env->vsr[xT(opcode)];                                   
> \
> +    ppc_vsr_t *xa = &env->vsr[xA(opcode)];                                   
> \
> +    ppc_vsr_t *xb = &env->vsr[xB(opcode)];                                   
> \
>      int i;                                                                   
> \
>                                                                               
> \
> -    getVSR(xA(opcode), &xa, env);                                            
> \
> -    getVSR(xB(opcode), &xb, env);                                            
> \
> -    getVSR(xT(opcode), &xt, env);                                            
> \
>      helper_reset_fpstatus(env);                                              
> \
>                                                                               
> \
>      for (i = 0; i < nels; i++) {                                             
> \
>          float_status tstat = env->fp_status;                                 
> \
>          set_float_exception_flags(0, &tstat);                                
> \
> -        xt.fld = tp##_##op(xa.fld, xb.fld, &tstat);                          
> \
> +        xt->fld = tp##_##op(xa->fld, xb->fld, &tstat);                       
> \
>          env->fp_status.float_exception_flags |= tstat.float_exception_flags; 
> \
>                                                                               
> \
>          if (unlikely(tstat.float_exception_flags & float_flag_invalid)) {    
> \
>              float_invalid_op_addsub(env, sfprf, GETPC(),                     
> \
> -                                    tp##_classify(xa.fld) |                  
> \
> -                                    tp##_classify(xb.fld));                  
> \
> +                                    tp##_classify(xa->fld) |                 
> \
> +                                    tp##_classify(xb->fld));                 
> \
>          }                                                                    
> \

Similarly.  Only here it's more interesting in that element 0 is modified when
element 3 raises an exception.  To keep current semantics you need to keep xt
as a ppc_vsr_t local variable and write back at the end.

It looks like the same is true for every other function.


r~