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

[David Hildenbrand]

On 03.06.21 20:13, Richard Henderson wrote:
> 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.

Will do, thanks.

> > +    } 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.

Thinking about it, that makes sense. I have no clue why the PoP has 
these special cases expressed in the tables, at least it managed to 
confuse me.

> > +    } 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

Another case of "let's make the tables inconsistent to confuse David".

>       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.

Agreed, thanks!


--
Thanks,

David / dhildenb