Re: Re: [PATCH v4 2/4] target/s390x: Emulate CVB, CVBY and CVBG

[Ilya Leoshkevich]

On Mon, Feb 05, 2024 at 06:04:27PM +0100, Thomas Huth wrote:
> On 02/02/2024 15.11, Ilya Leoshkevich wrote:
> > Convert to Binary - counterparts of the already implemented Convert
> > to Decimal (CVD*) instructions.
> > Example from the Principles of Operation: 25594C becomes 63FA.
> > 
> > Co-developed-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   target/s390x/helper.h            |  2 +
> >   target/s390x/tcg/insn-data.h.inc |  4 ++
> >   target/s390x/tcg/int_helper.c    | 72 ++++++++++++++++++++++++++++++++
> >   target/s390x/tcg/translate.c     | 16 +++++++
> >   4 files changed, 94 insertions(+)

[...]

> > +uint64_t HELPER(cvbg)(CPUS390XState *env, Int128 dec)
> > +{
> > +    uint64_t dec64[] = {int128_getlo(dec), int128_gethi(dec)};
> > +    int64_t bin = 0, pow10, tmp;
> > +    int digit, i, sign;
> > +
> > +    sign = dec64[0] & 0xf;
> > +    if (sign < 0xa) {
> > +        tcg_s390_data_exception(env, 0, GETPC());
> > +    }
> > +    dec64[0] >>= 4;
> > +    pow10 = (sign == 0xb || sign == 0xd) ? -1 : 1;
> > +
> > +    for (i = 1; i < 20; i++) {
> > +        digit = dec64[i >> 4] & 0xf;
> > +        if (digit > 0x9) {
> > +            tcg_s390_data_exception(env, 0, GETPC());
> > +        }
> > +        dec64[i >> 4] >>= 4;
> > +        tmp = pow10 * digit;
> > +        if (digit && ((tmp ^ pow10) < 0)) {
> 
> That tmp ^ pow10 caused some frowning for me first, but it's just a check
> whether the sign changed, right? ... a comment in front of the line might be
> helpful.

Good point about writing a comment, I tried to elaborate why checking
the sign is justified, and realized that it's actually redundant.
The int64_t bounds are roughly +-9.2E+18, and the last pow10 value in
this loop is +-1E+18. The multiplication cannot overflow.

> > +            tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
> > +        }
> > +        tmp = bin + tmp;
> > +        if (bin && ((tmp ^ bin) < 0)) {

The addition, however, can, e.g., bin=9E+17 and tmp=9E+18.
So I'll send a v5 without the first check and with a comment.

[...]