Re: [PATCH v1 04/20] s390x/tcg: Implement 32/128 bit for VECTOR FP ADD

[Richard Henderson]

On 9/30/20 9:55 AM, David Hildenbrand wrote:
> -typedef uint64_t (*vop64_3_fn)(uint64_t a, uint64_t b, float_status *s);
> -static void vop64_3(S390Vector *v1, const S390Vector *v2, const S390Vector 
> *v3,
> -                    CPUS390XState *env, bool s, vop64_3_fn fn,
> -                    uintptr_t retaddr)
> -{
> -    uint8_t vxc, vec_exc = 0;
> -    S390Vector tmp = {};
> -    int i;
> -
> -    for (i = 0; i < 2; i++) {
> -        const uint64_t a = s390_vec_read_element64(v2, i);
> -        const uint64_t b = s390_vec_read_element64(v3, i);
> -
> -        s390_vec_write_element64(&tmp, i, fn(a, b, &env->fpu_status));
> -        vxc = check_ieee_exc(env, i, false, &vec_exc);
> -        if (s || vxc) {
> -            break;
> -        }
> -    }
> -    handle_ieee_exc(env, vxc, vec_exc, retaddr);
> -    *v1 = tmp;
> -}
...
> +#define DEF_VOP_3(BITS)                                                      
>   \
> +typedef float##BITS (*vop##BITS##_3_fn)(float##BITS a, float##BITS b,        
>   \
> +                                        float_status *s);                    
>   \
> +static void vop##BITS##_3(S390Vector *v1, const S390Vector *v2,              
>   \
> +                          const S390Vector *v3, CPUS390XState *env, bool s,  
>   \
> +                          vop##BITS##_3_fn fn, uintptr_t retaddr)            
>   \
> +{                                                                            
>   \
> +    uint8_t vxc, vec_exc = 0;                                                
>   \
> +    S390Vector tmp = {};                                                     
>   \
> +    int i;                                                                   
>   \
> +                                                                             
>   \
> +    for (i = 0; i < (128 / BITS); i++) {                                     
>   \
> +        const float##BITS a = s390_vec_read_float##BITS(v2, i);              
>   \
> +        const float##BITS b = s390_vec_read_float##BITS(v3, i);              
>   \
> +                                                                             
>   \
> +        s390_vec_write_float##BITS(&tmp, i, fn(a, b, &env->fpu_status));     
>   \
> +        vxc = check_ieee_exc(env, i, false, &vec_exc);                       
>   \
> +        if (s || vxc) {                                                      
>   \
> +            break;                                                           
>   \
> +        }                                                                    
>   \
> +    }                                                                        
>   \
> +    handle_ieee_exc(env, vxc, vec_exc, retaddr);                             
>   \
> +    *v1 = tmp;                                                               
>   \
> +}
> +DEF_VOP_3(32)
> +DEF_VOP_3(64)
> +DEF_VOP_3(128)

While this works, you won't be able to step through this function in the
debugger anymore, because it now has one source line: at the point of expansion.

We do have plenty of these around the code base, I know.  This is small enough
that I think it's reasonable to simply have three copies, one for each type.

> +#define DEF_GVEC_FVA(BITS)                                                   
>   \
> +void HELPER(gvec_vfa##BITS)(void *v1, const void *v2, const void *v3,        
>   \
> +                            CPUS390XState *env, uint32_t desc)               
>   \
> +{                                                                            
>   \
> +    vop##BITS##_3(v1, v2, v3, env, false, float##BITS##_add, GETPC());       
>   \
> +}
> +DEF_GVEC_FVA(32)
> +DEF_GVEC_FVA(64)
> +DEF_GVEC_FVA(128)
> +
> +#define DEF_GVEC_FVA_S(BITS)                                                 
>   \
> +void HELPER(gvec_vfa##BITS##s)(void *v1, const void *v2, const void *v3,     
>   \
> +                               CPUS390XState *env, uint32_t desc)            
>   \
> +{                                                                            
>   \
> +    vop##BITS##_3(v1, v2, v3, env, true, float##BITS##_add, GETPC());        
>   \
> +}
> +DEF_GVEC_FVA_S(32)
> +DEF_GVEC_FVA_S(64)
I think you're defining these macros with the wrong parameters.  Think of how
to use the same macros for all of add/sub/etc.

E.g.

#define DEF_FOP3_B(NAME, OP, BITS) \...
  void HELPER(gvec_##NAME##BITS)(void *v1, const void *v2,
      const void *v3, CPUS390XState *env, uint32_t desc)
  {
    vop##BITS##_3(v1, v2, v3, env, false,
                  float##BITS##_##OP, GETPC());
  }
  void HELPER(gvec_##NAME##BITS##s)(void *v1, const void *v2,
      const void *v3, CPUS390XState *env, uint32_t desc)
  {
    vop##BITS##_3(v1, v2, v3, env, true,
                  float##BITS##_##OP, GETPC());
  }

#define DEF_FOP3(NAME, OP) \
  DEF_FOP3_B(NAME, OP, 32) \
  DEF_FOP3_B(NAME, OP, 64) \
  DEF_FOP3_B(NAME, OP, 128)

DEF_FOP3(vfa, add)
DEF_FOP3(vfd, div)
DEF_FOP3(vfm, mul)

etc.


r~