From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1LBqRN-0004eN-Iy
	for qemu-devel@nongnu.org; Sun, 14 Dec 2008 07:51:21 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1LBqRJ-0004Xf-Oh
	for qemu-devel@nongnu.org; Sun, 14 Dec 2008 07:51:21 -0500
Received: from [199.232.76.173] (port=34701 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1LBqRJ-0004XV-K9
	for qemu-devel@nongnu.org; Sun, 14 Dec 2008 07:51:17 -0500
Received: from mx20.gnu.org ([199.232.41.8]:18603)
	by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32)
	(Exim 4.60) (envelope-from <froydnj@codesourcery.com>)
	id 1LBqRJ-0004kG-Ab
	for qemu-devel@nongnu.org; Sun, 14 Dec 2008 07:51:17 -0500
Received: from mail.codesourcery.com ([65.74.133.4])
	by mx20.gnu.org with esmtp (Exim 4.60)
	(envelope-from <froydnj@codesourcery.com>) id 1LBqRH-00088t-UP
	for qemu-devel@nongnu.org; Sun, 14 Dec 2008 07:51:16 -0500
Date: Sun, 14 Dec 2008 04:51:14 -0800
From: Nathan Froyd <froydnj@codesourcery.com>
Subject: Re: [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions
Message-ID: <20081214125114.GH23471@codesourcery.com>
References: <20081214025628.GE23471@codesourcery.com>
	<20081214103554.GG17729@volta.aurel32.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20081214103554.GG17729@volta.aurel32.net>
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: qemu-devel@nongnu.org

On Sun, Dec 14, 2008 at 11:35:54AM +0100, Aurelien Jarno wrote:
> > -uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
> > +void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
> >  {
> >      CPU_DoubleU farg1, farg2;
> >      uint32_t ret = 0;
> >      farg1.ll = arg1;
> >      farg2.ll = arg2;
> >  
> > -    if (unlikely(float64_is_signaling_nan(farg1.d) ||
> > -                 float64_is_signaling_nan(farg2.d))) {
> > -        /* sNaN comparison */
> > -        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > +    if (unlikely(float64_is_nan(farg1.d) ||
> > +                 float64_is_nan(farg2.d))) {
> > +        ret = 0x01UL;
> > +    } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> > +        ret = 0x08UL;
> > +    } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> > +        ret = 0x04UL;
> >      } else {
> > -        if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> > -            ret = 0x08UL;
> > -        } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> > -            ret = 0x04UL;
> > -        } else {
> > -            ret = 0x02UL;
> > -        }
> > +        ret = 0x02UL;
> >      }
> > +
> >      env->fpscr &= ~(0x0F << FPSCR_FPRF);
> >      env->fpscr |= ret << FPSCR_FPRF;
> > -    return ret;
> > +    env->crf[crfD] = ret;
> > +    if (unlikely(ret == 0x01UL
> 
> Why do you need to test if the result is 0x01UL?
> 
> > +                 && (float64_is_signaling_nan(farg1.d) ||
> > +                     float64_is_signaling_nan(farg2.d)))) {
> 
> If at least one of the arguments is a sNaN, the result should already by
> 0x01.

My reasoning was that testing for NaNness would be relatively expensive
and that NaN input should be infrequent.  Testing for 0x01UL would
therefore be a quick check to see if we need to do more expensive
checks.  I'm happy to redo the patch in the interest of clarity.

> > +    if (unlikely (ret == 0x01UL)) {
> >          if (float64_is_signaling_nan(farg1.d) ||
> >              float64_is_signaling_nan(farg2.d)) {
> >              /* sNaN comparison */
> 
> Same here.

Ditto.

-Nathan