Re: [Qemu-devel] [PATCH v2 06/22] s390x/tcg: Implement VECTOR FP COMPARE (EQUAL|HIGH|HIGH OR EQUAL)

[Richard Henderson <richard.henderson@linaro.org>]

On 6/5/19 4:19 AM, David Hildenbrand wrote:
> On 03.06.19 11:06, David Hildenbrand wrote:
>> Provide for all three instructions all four combinations of cc bit and
>> s bit.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/helper.h           |  12 ++++
>>  target/s390x/insn-data.def      |   6 ++
>>  target/s390x/translate_vx.inc.c |  51 ++++++++++++++++
>>  target/s390x/vec_fpu_helper.c   | 104 ++++++++++++++++++++++++++++++++
>>  4 files changed, 173 insertions(+)
>>
>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>> index d34d6802a6..33d3bacf74 100644
>> --- a/target/s390x/helper.h
>> +++ b/target/s390x/helper.h
>> @@ -254,6 +254,18 @@ DEF_HELPER_FLAGS_5(gvec_vfa64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>>  DEF_HELPER_FLAGS_5(gvec_vfa64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>>  DEF_HELPER_4(gvec_wfc64, void, cptr, cptr, env, i32)
>>  DEF_HELPER_4(gvec_wfk64, void, cptr, cptr, env, i32)
>> +DEF_HELPER_FLAGS_5(gvec_vfce64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_FLAGS_5(gvec_vfce64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_5(gvec_vfce64_cc, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_5(gvec_vfce64s_cc, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_FLAGS_5(gvec_vfch64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_FLAGS_5(gvec_vfch64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_5(gvec_vfch64_cc, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_5(gvec_vfch64s_cc, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_FLAGS_5(gvec_vfche64, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_FLAGS_5(gvec_vfche64s, TCG_CALL_NO_WG, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_5(gvec_vfche64_cc, void, ptr, cptr, cptr, env, i32)
>> +DEF_HELPER_5(gvec_vfche64s_cc, void, ptr, cptr, cptr, env, i32)
>>  
>>  #ifndef CONFIG_USER_ONLY
>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
>> index c45e101b10..446552f251 100644
>> --- a/target/s390x/insn-data.def
>> +++ b/target/s390x/insn-data.def
>> @@ -1212,6 +1212,12 @@
>>      F(0xe7cb, WFC,     VRR_a, V,   0, 0, 0, 0, wfc, 0, IF_VEC)
>>  /* VECTOR FP COMPARE AND SIGNAL SCALAR */
>>      F(0xe7ca, WFK,     VRR_a, V,   0, 0, 0, 0, wfc, 0, IF_VEC)
>> +/* VECTOR FP COMPARE EQUAL */
>> +    F(0xe7e8, VFCE,    VRR_c, V,   0, 0, 0, 0, vfc, 0, IF_VEC)
>> +/* VECTOR FP COMPARE HIGH */
>> +    F(0xe7eb, VFCH,    VRR_c, V,   0, 0, 0, 0, vfc, 0, IF_VEC)
>> +/* VECTOR FP COMPARE HIGH OR EQUAL */
>> +    F(0xe7ea, VFCHE,   VRR_c, V,   0, 0, 0, 0, vfc, 0, IF_VEC)
>>  
>>  #ifndef CONFIG_USER_ONLY
>>  /* COMPARE AND SWAP AND PURGE */
>> diff --git a/target/s390x/translate_vx.inc.c b/target/s390x/translate_vx.inc.c
>> index 283e8aa07a..5571a71e1a 100644
>> --- a/target/s390x/translate_vx.inc.c
>> +++ b/target/s390x/translate_vx.inc.c
>> @@ -2588,3 +2588,54 @@ static DisasJumpType op_wfc(DisasContext *s, DisasOps *o)
>>      set_cc_static(s);
>>      return DISAS_NEXT;
>>  }
>> +
>> +static DisasJumpType op_vfc(DisasContext *s, DisasOps *o)
>> +{
>> +    const uint8_t fpf = get_field(s->fields, m4);
>> +    const uint8_t m5 = get_field(s->fields, m5);
>> +    const uint8_t m6 = get_field(s->fields, m6);
>> +    const bool se = extract32(m5, 3, 1);
>> +    const bool cs = extract32(m6, 0, 1);
>> +    gen_helper_gvec_3_ptr *fn;
>> +
>> +    if (fpf != FPF_LONG || extract32(m5, 0, 3) || extract32(m6, 1, 3)) {
>> +        gen_program_exception(s, PGM_SPECIFICATION);
>> +        return DISAS_NORETURN;
>> +    }
>> +
>> +    if (cs) {
>> +        switch (s->fields->op2) {
>> +        case 0xe8:
>> +            fn = se ? gen_helper_gvec_vfce64s_cc : gen_helper_gvec_vfce64_cc;
>> +            break;
>> +        case 0xeb:
>> +            fn = se ? gen_helper_gvec_vfch64s_cc : gen_helper_gvec_vfch64_cc;
>> +            break;
>> +        case 0xea:
>> +            fn = se ? gen_helper_gvec_vfche64s_cc : gen_helper_gvec_vfche64_cc;
>> +            break;
>> +        default:
>> +            g_assert_not_reached();
>> +        }
>> +    } else {
>> +        switch (s->fields->op2) {
>> +        case 0xe8:
>> +            fn = se ? gen_helper_gvec_vfce64s : gen_helper_gvec_vfce64;
>> +            break;
>> +        case 0xeb:
>> +            fn = se ? gen_helper_gvec_vfch64s : gen_helper_gvec_vfch64;
>> +            break;
>> +        case 0xea:
>> +            fn = se ? gen_helper_gvec_vfche64s : gen_helper_gvec_vfche64;
>> +            break;
>> +        default:
>> +            g_assert_not_reached();
>> +        }
>> +    }
>> +    gen_gvec_3_ptr(get_field(s->fields, v1), get_field(s->fields, v2),
>> +                   get_field(s->fields, v3), cpu_env, 0, fn);
>> +    if (cs) {
>> +        set_cc_static(s);
>> +    }
>> +    return DISAS_NEXT;
>> +}
>> diff --git a/target/s390x/vec_fpu_helper.c b/target/s390x/vec_fpu_helper.c
>> index f9357d9221..e72500d4d5 100644
>> --- a/target/s390x/vec_fpu_helper.c
>> +++ b/target/s390x/vec_fpu_helper.c
>> @@ -149,3 +149,107 @@ void HELPER(gvec_wfk64)(const void *v1, const void *v2, CPUS390XState *env,
>>  {
>>      env->cc_op = wfc64(v1, v2, env, true, GETPC());
>>  }
>> +
>> +typedef int (*vfc64_fn)(float64 a, float64 b, float_status *status);
>> +static int vfc64(S390Vector *v1, const S390Vector *v2, const S390Vector *v3,
>> +                 CPUS390XState *env, bool s, vfc64_fn fn, uintptr_t retaddr)
>> +{
>> +    uint8_t vxc, vec_exc = 0;
>> +    S390Vector tmp = {};
>> +    int match = 0;
>> +    int i;
>> +
>> +    for (i = 0; i < 2; i++) {
>> +        const float64 a = s390_vec_read_element64(v2, i);
>> +        const float64 b = s390_vec_read_element64(v3, i);
>> +
>> +        /* swap the order of the parameters, so we can use existing functions */
>> +        if (fn(b, a, &env->fpu_status)) {
>> +            match++;
>> +            s390_vec_write_element64(&tmp, i, -1ull);
>> +        }
>> +        vxc = check_ieee_exc(env, i, false, &vec_exc);
>> +        if (s || vxc) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    handle_ieee_exc(env, vxc, vec_exc, retaddr);
>> +    *v1 = tmp;
>> +    if (match) {
>> +        return s || match == 2 ? 0 : 1;
>> +    }
>> +    return 3;
>> +}
>> +
>> +void HELPER(gvec_vfce64)(void *v1, const void *v2, const void *v3,
>> +                         CPUS390XState *env, uint32_t desc)
>> +{
>> +    vfc64(v1, v2, v3, env, false, float64_eq_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfce64s)(void *v1, const void *v2, const void *v3,
>> +                          CPUS390XState *env, uint32_t desc)
>> +{
>> +    vfc64(v1, v2, v3, env, true, float64_eq_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfce64_cc)(void *v1, const void *v2, const void *v3,
>> +                            CPUS390XState *env, uint32_t desc)
>> +{
>> +    env->cc_op = vfc64(v1, v2, v3, env, false, float64_eq_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfce64s_cc)(void *v1, const void *v2, const void *v3,
>> +                            CPUS390XState *env, uint32_t desc)
>> +{
>> +    env->cc_op = vfc64(v1, v2, v3, env, true, float64_eq_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfch64)(void *v1, const void *v2, const void *v3,
>> +                         CPUS390XState *env, uint32_t desc)
>> +{
>> +    vfc64(v1, v2, v3, env, false, float64_lt_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfch64s)(void *v1, const void *v2, const void *v3,
>> +                          CPUS390XState *env, uint32_t desc)
>> +{
>> +    vfc64(v1, v2, v3, env, true, float64_lt_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfch64_cc)(void *v1, const void *v2, const void *v3,
>> +                            CPUS390XState *env, uint32_t desc)
>> +{
>> +    env->cc_op = vfc64(v1, v2, v3, env, false, float64_lt_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfch64s_cc)(void *v1, const void *v2, const void *v3,
>> +                             CPUS390XState *env, uint32_t desc)
>> +{
>> +    env->cc_op = vfc64(v1, v2, v3, env, true, float64_lt_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfche64)(void *v1, const void *v2, const void *v3,
>> +                          CPUS390XState *env, uint32_t desc)
>> +{
>> +    vfc64(v1, v2, v3, env, false, float64_le_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfche64s)(void *v1, const void *v2, const void *v3,
>> +                           CPUS390XState *env, uint32_t desc)
>> +{
>> +    vfc64(v1, v2, v3, env, true, float64_le_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfche64_cc)(void *v1, const void *v2, const void *v3,
>> +                             CPUS390XState *env, uint32_t desc)
>> +{
>> +    env->cc_op = vfc64(v1, v2, v3, env, false, float64_le_quiet, GETPC());
>> +}
>> +
>> +void HELPER(gvec_vfche64s_cc)(void *v1, const void *v2, const void *v3,
>> +                              CPUS390XState *env, uint32_t desc)
>> +{
>> +    env->cc_op = vfc64(v1, v2, v3, env, true, float64_le_quiet, GETPC());
>> +}
>>
> 
> Richard, still want to review this patch? I'm planning to send a pull
> request soon.
> 

Somehow I bypassed this one.  Anyway,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~