From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50596) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VZh1t-0001hw-5e for qemu-devel@nongnu.org; Fri, 25 Oct 2013 09:02:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VZh1n-0005xZ-0U for qemu-devel@nongnu.org; Fri, 25 Oct 2013 09:02:17 -0400 Message-ID: <526A6BA8.1000001@gmail.com> Date: Fri, 25 Oct 2013 08:01:28 -0500 From: Tom Musta MIME-Version: 1.0 References: <526947CA.4020504@gmail.com> <52694821.4040001@gmail.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/19] Add New softfloat Routines for VSX List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "qemu-ppc@nongnu.org" , QEMU Developers Peter: Thanks for your feedback. Responses below. On 10/25/2013 6:55 AM, Peter Maydell wrote: > On 24 October 2013 17:17, Tom Musta wrote: >> This patch adds routines to the softfloat library that are useful for >> the PowerPC VSX implementation. The routines are, however, not specific >> to PowerPC and are approprriate for softfloat. >> >> The following routines are added: >> >> - float32_is_denormal() returns true if the 32-bit floating point number >> is denormalized. >> - float64_is_denormal() returns true if the 64-bit floating point number >> is denormalized. > > Can you point me at the patches which use these, please? > I couldn't find them with a quick search in my email client. Please see http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html > >> - float32_get_unbiased_exp() returns the unbiased exponent of a 32-bit >> floating point number. >> - float64_get_unbiased_exp() returns the unbiased exponent of a 64-bit >> floating point number. > > These look rather odd to me, and again I can't find the uses in > your patchset. Returning just the exponent is a bit odd and > suggests that maybe the split between target code and softfloat > is in the wrong place. Please see http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html and http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03107.html and also the corresponding definitions of those instructions in the Power ISA. What is odd here is the PowerPC instruction(s) :) But given that softfloat code extracts exponents in numerous places, I do not find it odd at all that a floating point instruction model for a non-standard operation might have to do the same. These functions can easily be kept within the PowerPC code proper if there are objections to them being added to softfloat. I would rename them, of course, so that they do not look like softfloat routines. >> - float32_to_uint64() converts a 32-bit floating point number to an >> unsigned 64 bit number. > > I would put this in its own patch, personally. Fair enough. Just so that I am clear ... do you mean submit this as a patch just by itself (not as part of a series of VSX additions)? >> >> +INLINE int float32_is_denormal(float32 a) >> +{ >> + return ((float32_val(a) & 0x7f800000) == 0) && >> + ((float32_val(a) & 0x007fffff) != 0); >> +} > > return float32_is_zero_or_denormal(a) && !float32_is_zero(a); > > is easier to review and less duplicative of code. > > thanks It surprised me that there were is_zero and is_zero_or_denormal functions but not is_denormal functions. I would find it more normal to implement the two primitive functions and then construct is_zero_or_denormal to be the OR of those two. Until you look at efficiency of the implementation.