From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:41659)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <aurelien@aurel32.net>) id 1TOGWT-0003O3-5b
	for qemu-devel@nongnu.org; Tue, 16 Oct 2012 19:26:06 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <aurelien@aurel32.net>) id 1TOGWR-0005be-Pp
	for qemu-devel@nongnu.org; Tue, 16 Oct 2012 19:26:05 -0400
Received: from hall.aurel32.net ([88.191.126.93]:52893)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <aurelien@aurel32.net>) id 1TOGWR-0005b3-JC
	for qemu-devel@nongnu.org; Tue, 16 Oct 2012 19:26:03 -0400
Date: Wed, 17 Oct 2012 01:26:01 +0200
From: Aurelien Jarno <aurelien@aurel32.net>
Message-ID: <20121016232601.GA25878@ohm.aurel32.net>
References: <1349814458-21739-1-git-send-email-aurelien@aurel32.net>
	<1349814458-21739-5-git-send-email-aurelien@aurel32.net>
	<5075D60D.5090700@twiddle.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-15
Content-Disposition: inline
In-Reply-To: <5075D60D.5090700@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH 04/14] target-mips: use softfloat constants
 when possible
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Richard Henderson <rth@twiddle.net>
Cc: qemu-devel@nongnu.org

On Wed, Oct 10, 2012 at 01:09:49PM -0700, Richard Henderson wrote:
> On 10/09/2012 01:27 PM, Aurelien Jarno wrote:
> > softfloat already has a few constants defined, use them instead of
> > redefining them in target-mips.
> > 
> > Rename FLOAT_SNAN32 and FLOAT_SNAN64 to FP_TO_INT32_OVERFLOW and
> > FP_TO_INT64_OVERFLOW as even if they have the same value, they are
> > technically different (and defined differently in the MIPS ISA).
> > 
> > Remove the unused constants.
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> 
> > @@ -2495,8 +2491,9 @@ uint64_t helper_float_cvtl_d(CPUMIPSState *env, uint64_t fdt0)
> >      set_float_exception_flags(0, &env->active_fpu.fp_status);
> >      dt2 = float64_to_int64(fdt0, &env->active_fpu.fp_status);
> >      update_fcr31(env);
> > -    if (GET_FP_CAUSE(env->active_fpu.fcr31) & (FP_OVERFLOW | FP_INVALID))
> > -        dt2 = FLOAT_SNAN64;
> > +    if (GET_FP_CAUSE(env->active_fpu.fcr31) & (FP_OVERFLOW | FP_INVALID)) {
> > +        dt2 = FP_TO_INT64_OVERFLOW;
> > +    }
> >      return dt2;
> 
> That said, the existing code you're patching is incorrect.
> 
> This code will fold to OVERFLOW if any previous operation caused an overflow,
> not checking that the *current* operation caused an overflow.
> 

While I agree it should check the softfloat flags instead, I disagree it
is wrong. The part that GET_FP_CAUSE() is looking at is not the
accumulated flags, but the flags for the last instruction.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net