From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VgbXY-0004Zr-De for qemu-devel@nongnu.org; Wed, 13 Nov 2013 09:35:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VgbXP-0007Dc-Q2 for qemu-devel@nongnu.org; Wed, 13 Nov 2013 09:35:32 -0500 Message-ID: <52838E26.8030600@gmail.com> Date: Wed, 13 Nov 2013 08:35:18 -0600 From: Tom Musta MIME-Version: 1.0 References: <1383769916-5582-1-git-send-email-tommusta@gmail.com> <527C2F10.7040701@twiddle.net> In-Reply-To: <527C2F10.7040701@twiddle.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 00/14] VSX Stage 4 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org On 11/7/2013 6:23 PM, Richard Henderson wrote: > Modulo my comments wrt the actual computation of fma, the patches all look fine. > > But I'd like to also mention a pre-existing flaw/niggle in the ppc port. > > The conversions to/from in-register representation for the single-precision > values should never raise exceptions. Yet we always use > > d.d = float32_to_float64(f.f, &env->fp_status); > f.f = float64_to_float32(d.d, &env->fp_status); > > The use of env->fp_status is either wrong or extremely misleading. It sure > looks like the operation affects global cpu state. It may be that that state > is never copied back to the "real" fpscr and so doesn't actually affect cpu > state, but how can I see that for sure? > > I think it would be better to implement ConvertSPtoDP_NP and ConvertSP64toSP > exactly as written in the spec. > > Or at minimum use a dummy fp_status that's not associated with env. It should > not matter what the "real" rounding mode is in either case, since values that > are not exactly representable as single-precision values give undefined results. I've looked more closely at the code and have performed some experiments. There are several status flags that are being set by the float32_to_float64 call. And they are copied back near the end of these routines via the helper_float_check_status. So I think this is all necessary. That said, rather than repeating the float32_to_float64() / float64_to_float32() pattern everywhere, I should have reused the existing helper_frsp() routine. So I will be publishing a V2.