On 24 March 2011 15:58, Alexander Graf <ag...@suse.de> 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.
> +/* 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).
> +/* 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.
> +/* 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?
> +/* 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?
> +/* 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...
> +/* 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.
> +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.
> +static inline uint32_t cc_calc_nabs_32(CPUState *env, int32_t dst)
> +{
> + return !!dst;
> +}
Another candidate for inlining.
-- PMM
