On Mon, 19 Jul 2021 at 16:28, Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Sat, 17 Jul 2021 at 21:46, Richard Henderson > <richard.hender...@linaro.org> wrote: > > > > On 7/13/21 6:37 AM, Peter Maydell wrote: > > > +/* Max and min of absolute values */ > > > +static int64_t do_maxa(int64_t n, int64_t m) > > > +{ > > > + if (n < 0) { > > > + n = -n; > > > + } > > > + if (m < 0) { > > > + m = -m; > > > + } > > > + return MAX(n, m); > > > +} > > > > This doesn't look quite right. The n operand is extracted unsigned, and > > only the m > > operand is subjected to ABS. > > Yes, you're right (this patch does always extract ra (here n) > unsigned, so the only case where it makes a difference is > VMINAV when the input ra is negative). I noticed this wrinkle > when implementing the FP variant of this insn, but missed it here.
In fact it doesn't make a difference for this integer version: because we always extract n as an unsigned value, 'n' is always positive (either because it's the initial unsigned 'ra' or because it's the result of a previous do_maxa/do_mina, which must also always be positive) and so taking the absolute value of 'n' never affects it. But we might as well delete the unused code. (For float it *does* make a difference, because there's no "extract as unsigned" for floats, and so the input 'ra' and thus the first 'n' can be negative there.) -- PMM