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.zbits...@gmail.com> > > Signed-off-by: Ilya Leoshkevich <i...@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. [...]