From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=47559 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q4GAW-0007Wq-TO for qemu-devel@nongnu.org; Mon, 28 Mar 2011 13:23:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q4GAV-0003jF-7d for qemu-devel@nongnu.org; Mon, 28 Mar 2011 13:23:56 -0400 Received: from cantor.suse.de ([195.135.220.2]:42680 helo=mx1.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q4GAU-0003iz-Pi for qemu-devel@nongnu.org; Mon, 28 Mar 2011 13:23:55 -0400 Message-ID: <4D90C428.2030101@suse.de> Date: Mon, 28 Mar 2011 19:23:52 +0200 From: Alexander Graf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 14/17] s390x: Implement opcode helpers References: <1300982333-12802-1-git-send-email-agraf@suse.de> <1300982333-12802-15-git-send-email-agraf@suse.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU-devel Developers , Aurelien Jarno , Richard Henderson On 03/24/2011 06:29 PM, Peter Maydell wrote: > On 24 March 2011 15:58, Alexander Graf wrote: > > This is more random comments in passing than a thorough review; sorry. > >> +#if HOST_LONG_BITS == 64&& defined(__GNUC__) >> + /* assuming 64-bit hosts have __uint128_t */ >> + __uint128_t dividend = (((__uint128_t)env->regs[r1])<< 64) | >> + (env->regs[r1+1]); >> + __uint128_t quotient = dividend / divisor; >> + env->regs[r1+1] = quotient; >> + __uint128_t remainder = dividend % divisor; >> + env->regs[r1] = remainder; >> +#else >> + /* 32-bit hosts would need special wrapper functionality - just abort if >> + we encounter such a case; it's very unlikely anyways. */ >> + cpu_abort(env, "128 -> 64/64 division not implemented\n"); >> +#endif > ...I'm still using a 32 bit system :-) > >> +/* condition codes for binary FP ops */ >> +static uint32_t set_cc_f32(float32 v1, float32 v2) >> +{ >> + if (float32_is_any_nan(v1) || float32_is_any_nan(v2)) { >> + return 3; >> + } else if (float32_eq(v1, v2,&env->fpu_status)) { >> + return 0; >> + } else if (float32_lt(v1, v2,&env->fpu_status)) { >> + return 1; >> + } else { >> + return 2; >> + } >> +} > Can you not use float32_compare_quiet() (returns a value > telling you if it's less/equal/greater/unordered)? > If not, needs a comment saying why you need to do it the hard way. I just checked the macros there and it looks like float32_compare_quiet returns eq when both numbers are NaN. We would still have to convert from the return value from that over to a CC value. I honestly don't see any benefit - the code doesn't become cleaner or smaller. int float32_compare_quiet( float32 a, float32 b STATUS_PARAM ) { if (isless(a, b)) { return float_relation_less; } else if (a == b) { return float_relation_equal; } else if (isgreater(a, b)) { return float_relation_greater; } else { return float_relation_unordered; } } so static uint32_t set_cc_f32(float32 v1, float32 v2) { if (float32_is_any_nan(v1) || float32_is_any_nan(v2)) { return 3; } else if (float32_eq(v1, v2, &env->fpu_status)) { return 0; } else if (float32_lt(v1, v2, &env->fpu_status)) { return 1; } else { return 2; } } would become static uint32_t set_cc_f32(float32 v1, float32 v2) { int r; if (float32_is_any_nan(v1) || float32_is_any_nan(v2)) { return 3; } r = float32_compare_quiet(v1, v2, &env->fpu_status); switch (r) { case float_relation_equal: return 0; case float_relation_less: return 1; default: return 2; } } >> +/* negative absolute of 32-bit float */ >> +uint32_t HELPER(lcebr)(uint32_t f1, uint32_t f2) >> +{ >> + env->fregs[f1].l.upper = float32_sub(float32_zero, env->fregs[f2].l.upper, >> +&env->fpu_status); >> + return set_cc_nz_f32(env->fregs[f1].l.upper); >> +} > Google suggests this is wrong: > http://publib.boulder.ibm.com/cgi-bin/bookmgr/BOOKS/DZ9AR006/19.4.10?SHELF=&DT=19990630131355&CASE= > says for lcebr that: > "The sign is inverted for any operand, including a QNaN or SNaN, > without causing an arithmetic exception." > > but float32_sub will raise exceptions for NaNs. You want > float32_chs() (and similarly for the other types). Ah, nice :). Thanks! >> +/* convert 64-bit float to 128-bit float */ >> +uint32_t HELPER(lcxbr)(uint32_t f1, uint32_t f2) > Wrong comment? Looks like another invert-sign op from > the online POO. Yup, wrong comment and the same as above for chs :). >> +/* 128-bit FP compare RR */ >> +uint32_t HELPER(cxbr)(uint32_t f1, uint32_t f2) >> +{ >> + CPU_QuadU v1; >> + v1.ll.upper = env->fregs[f1].ll; >> + v1.ll.lower = env->fregs[f1 + 2].ll; >> + CPU_QuadU v2; >> + v2.ll.upper = env->fregs[f2].ll; >> + v2.ll.lower = env->fregs[f2 + 2].ll; >> + if (float128_is_any_nan(v1.q) || float128_is_any_nan(v2.q)) { >> + return 3; >> + } else if (float128_eq(v1.q, v2.q,&env->fpu_status)) { >> + return 0; >> + } else if (float128_lt(v1.q, v2.q,&env->fpu_status)) { >> + return 1; >> + } else { >> + return 2; >> + } >> +} > float128_compare_quiet() again? See above :) >> +/* convert 32-bit float to 64-bit int */ >> +uint32_t HELPER(cgebr)(uint32_t r1, uint32_t f2, uint32_t m3) >> +{ >> + float32 v2 = env->fregs[f2].l.upper; >> + set_round_mode(m3); >> + env->regs[r1] = float32_to_int64(v2,&env->fpu_status); >> + return set_cc_nz_f32(v2); >> +} > Should this really be permanently setting the rounding mode > for future instructions as well as for the op it does itself? IIUC every op that does rounding sets the rounding mode, no? >> +/* load 32-bit FP zero */ >> +void HELPER(lzer)(uint32_t f1) >> +{ >> + env->fregs[f1].l.upper = float32_zero; >> +} > Surely this is trivial enough to inline rather than > calling a helper function... Lots of the FPU code could use inlining. The CC stuff does too. For now, I kept things the way Uli wrote them :). >> +/* load 128-bit FP zero */ >> +void HELPER(lzxr)(uint32_t f1) >> +{ >> + CPU_QuadU x; >> + x.q = float64_to_float128(float64_zero,&env->fpu_status); > Yuck. Just define a float128_zero if we need one. Good point. Mind to do so? I find myself struggling with the code there. >> +uint32_t HELPER(tceb)(uint32_t f1, uint64_t m2) >> +{ >> + float32 v1 = env->fregs[f1].l.upper; >> + int neg = float32_is_neg(v1); >> + uint32_t cc = 0; >> + >> + HELPER_LOG("%s: v1 0x%lx m2 0x%lx neg %d\n", __FUNCTION__, (long)v1, m2, neg); >> + if ((float32_is_zero(v1)&& (m2& (1<< (11-neg)))) || >> + (float32_is_infinity(v1)&& (m2& (1<< (5-neg)))) || >> + (float32_is_any_nan(v1)&& (m2& (1<< (3-neg)))) || >> + (float32_is_signaling_nan(v1)&& (m2& (1<< (1-neg))))) { >> + cc = 1; >> + } else if (m2& (1<< (9-neg))) { >> + /* assume normalized number */ >> + cc = 1; >> + } >> + >> + /* FIXME: denormalized? */ >> + return cc; >> +} > There's a float32_is_zero_or_denormal(); if you need a > float32_is_denormal() which is false for real zero we > could add it, I guess. Good point for a follow-up :) >> +static inline uint32_t cc_calc_nabs_32(CPUState *env, int32_t dst) >> +{ >> + return !!dst; >> +} > Another candidate for inlining. Same as above :) Alex