Re: [PATCH v2 24/26] s390x/tcg: Implement VECTOR FP (MAXIMUM|MINIMUM)

[Richard Henderson]

On 5/17/21 7:27 AM, David Hildenbrand wrote:
> +    if (unlikely(nan_a || nan_b)) {

Perhaps better as (dcmask_a | dcmask_b) & DCMASK_NAN ?

> +        if (sig_a || sig_b) {

Similarly.

> +    } else if (unlikely(inf_a && inf_b)) {
> +        switch (type) {
> +        case S390_MINMAX_TYPE_JAVA:
> +            return neg_a && !neg_b ? S390_MINMAX_RES_A : S390_MINMAX_RES_B;
> +        case S390_MINMAX_TYPE_C_MACRO:
> +        case S390_MINMAX_TYPE_CPP:
> +            return neg_b ? S390_MINMAX_RES_B : S390_MINMAX_RES_A;
> +        case S390_MINMAX_TYPE_F:
> +            return !neg_a && neg_b ? S390_MINMAX_RES_B : S390_MINMAX_RES_A;
> +        default:
> +            g_assert_not_reached();
> +        }

I don't understand why inf_a && inf_b gets a special case.  Inf is 
well-ordered. If the arguments are equal you can't tell the difference between 
them, so it doesn't matter whether A or B is returned.

I would pass this case along to S390_MINMAX_RES_MINMAX.

> +    } else if (unlikely(zero_a && zero_b)) {
> +        switch (type) {
> +        case S390_MINMAX_TYPE_JAVA:
> +            return neg_a && !neg_b ? S390_MINMAX_RES_A : S390_MINMAX_RES_B;

If neg_a && neg_b, both A and B are -0, and you can't distinguish them.  So 
this would seem to simplify to

    neg_a ? S390_MINMAX_RES_A : S390_MINMAX_RES_B

> +        case S390_MINMAX_TYPE_C_MACRO:
> +            return S390_MINMAX_RES_B;
> +        case S390_MINMAX_TYPE_F:
> +            return !neg_a && neg_b ? S390_MINMAX_RES_B : S390_MINMAX_RES_A;

Similarly if !neg_a && !neg_b, both A and B are +0.

Otherwise this looks good.


r~