Re: [PATCH v5 25/45] target/arm: Implement BFMOPA, BFMOPS

[Richard Henderson]

On 7/7/22 15:12, Peter Maydell wrote:
> > +static inline uint32_t f16mop_adj_pair(uint32_t pair, uint32_t pg, uint32_t 
> > neg)
> > +{
> > +    pair ^= neg;
>
> You seem to be negating element 1 of row and col ('neg' here is
> 1 << 15 unless I've misread something, and it gets passed to
> the calls for both the row and column data), but the pseudocode
> says we want to negate element 0 and element 1 of row, and not
> negate the col elements.

Yep, thanks.

> > +    if (!(pg & 1)) {
> > +        pair &= 0xffff0000u;
> > +    }
> > +    if (!(pg & 4)) {
> > +        pair &= 0x0000ffffu;
> > +    }
>
> The pseudocode sets the element to 0 if it is not
> predicated, and then applies the negation second.

Yes.  However, the negation is predicated too -- the squashed FPZero is never negated.  I 
found it simpler to unconditionally negate and then conditionally squash to zero.

> > +            uint32_t n = *(uint32_t *)(vzn + row);
>
> More missing H macros ?

Yep.

> > +                    if ((pa & 0b0101) == 0b0101 || (pb & 0b0101) == 0b0101) {
>
> The pseudocode test for "do we do anything" is
>   (prow_0 && pcol_0) || (prow_1 && pcol_1)
>
> but isn't this C expression doing
>   (prow_0 && prow_1) || (pcol_0 && pcol_1) ?

Yep, thanks.


r~