Re: [AD] 4.2 final cleanup

[Peter Wang <tjaden@xxxxxxxxxx>]

On 2005-09-24, Elias Pschernig <elias@xxxxxxxxxx> wrote:
> On Sat, 2005-09-24 at 19:10 +0100, Peter Hull wrote:
> 
> > 
> > Also, is this one (pivot_sprite, turned out to be something to do with 
> > the fixed point arithmetic) fixed: 
> > http://www.allegro.cc/forums/view_thread.php?_id=525743#target
> > 
> 
> Oh, I remember, I made a very simple fix for that.. but it seems the
> real problem was in some change that was made to fixed point routines,
> so I didn't commit it. If there's no other news, I'll commit that fix
> (just use a double instead of a fixed), since it shouldn't have any ba
> consequences.

The documentation for fixmul specifically talks about overflow, so we
need to restore that behaviour, or change the semantics and update the
documentation.

The following patch:

1. For Intel machines (and compilers without long long) revert to the
original implementation of fixmul (i.e. fixmulf).  I don't know how to
detect an overflow with only 32-bit registers, and when I checked it
again fixmulf was faster (on my P4) than fixmuli anyway!

2. For everything else, I made fixmull detect overflows.  It's much
slower as a result, but still faster than fixmulf on an AMD64.

Peter

Index: include/allegro/inline/fmaths.inl
===================================================================
RCS file: /cvsroot/alleg/allegro/include/allegro/inline/fmaths.inl,v
retrieving revision 1.9
diff -u -p -r1.9 fmaths.inl
--- include/allegro/inline/fmaths.inl	1 Jul 2005 02:52:16 -0000	1.9
+++ include/allegro/inline/fmaths.inl	25 Sep 2005 04:37:00 -0000
@@ -109,34 +109,37 @@ AL_INLINE(fixed, fixsub, (fixed x, fixed
  * Results varied with other compilers, optimisation levels, etc.
  * so this is not optimal, though a tenable compromise.
  *
+ * Note that the following implementation are NOT what were benchmarked.
+ * We had forgotten to put in overflow detection in those versions.
+ * If you don't need overflow detection then previous versions in the
+ * CVS tree might be worth looking at.
+ *
  * PS. Don't move the #ifs inside the AL_INLINE; BCC doesn't like it.
  */
 #if (defined ALLEGRO_I386) || (!defined LONG_LONG)
    AL_INLINE(fixed, fixmul, (fixed x, fixed y),
    {
-      fixed sign = (x^y) & 0x80000000;
-      int mask_x = x >> 31;
-      int mask_y = y >> 31;
-      int mask_result = sign >> 31;
-      fixed result;
-
-      x = (x^mask_x) - mask_x;
-      y = (y^mask_y) - mask_y;
-
-      result = ((y >> 8)*(x >> 8) +
-		(((y >> 8)*(x&0xff)) >> 8) +
-		(((x >> 8)*(y&0xff)) >> 8));
-
-      return (result^mask_result) - mask_result;
+      return ftofix(fixtof(x) * fixtof(y));
    })
 #else
    AL_INLINE(fixed, fixmul, (fixed x, fixed y),
    {
       LONG_LONG lx = x;
       LONG_LONG ly = y;
-      LONG_LONG lres = (lx*ly)>>16;
-      int res = lres;
-      return res;
+      LONG_LONG lres = (lx*ly);
+
+      if (lres > 0x7FFFFFFF0000LL) {
+	 *allegro_errno = ERANGE;
+	 return 0x7FFFFFFF;
+      }
+      else if (lres < -0x7FFFFFFF0000LL) {
+	 *allegro_errno = ERANGE;
+	 return 0x80000000;
+      }
+      else {
+	 int res = lres >> 16;
+	 return res;
+      }
    })
 #endif	    /* fixmul() C implementations */

Attachment: fixtime.c.gz
Description: application/gunzip