[PATCH] powerpc: Get rid of invalid shifts in math-emu

Signed-off-by: Segher Boessenkool <redacted>
---
 arch/powerpc/math-emu/op-2.h |   75 ++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/arch/powerpc/math-emu/op-2.h b/arch/powerpc/math-emu/op-2.h
index 7d6f17c..16d3e3c 100644
--- a/arch/powerpc/math-emu/op-2.h
+++ b/arch/powerpc/math-emu/op-2.h

@@ -11,58 +11,43 @@
 
 #define _FP_FRAC_SLL_2(X,N)						\
   do {									\
-    if ((N) < _FP_W_TYPE_SIZE)						\
+    int n = (N);							\
+    if (n >= _FP_W_TYPE_SIZE)						\
       {									\
-        if (__builtin_constant_p(N) && (N) == 1) 			\
-          {								\
-            X##_f1 = X##_f1 + X##_f1 + (((_FP_WS_TYPE)(X##_f0)) < 0);	\
-            X##_f0 += X##_f0;						\
-          }								\
-        else								\
-          {								\
-	    X##_f1 = X##_f1 << (N) | X##_f0 >> (_FP_W_TYPE_SIZE - (N));	\
-	    X##_f0 <<= (N);						\
-	  }								\
-      }									\
-    else								\
-      {									\
-	X##_f1 = X##_f0 << ((N) - _FP_W_TYPE_SIZE);			\
+	X##_f1 = X##_f0;						\
 	X##_f0 = 0;							\
+	n -= _FP_W_TYPE_SIZE;						\
       }									\
+    X##_f1 = X##_f1 << n | X##_f0 >> (_FP_W_TYPE_SIZE - n - 1) >> 1;	\
+    X##_f0 <<= n;							\
   } while (0)
 
 #define _FP_FRAC_SRL_2(X,N)						\
   do {									\
-    if ((N) < _FP_W_TYPE_SIZE)						\
-      {									\
-	X##_f0 = X##_f0 >> (N) | X##_f1 << (_FP_W_TYPE_SIZE - (N));	\
-	X##_f1 >>= (N);							\
-      }									\
-    else								\
+    int n = (N);							\
+    if (n >= _FP_W_TYPE_SIZE)						\
       {									\
-	X##_f0 = X##_f1 >> ((N) - _FP_W_TYPE_SIZE);			\
+	X##_f0 = X##_f1;						\
 	X##_f1 = 0;							\
+	n -= _FP_W_TYPE_SIZE;						\
       }									\
+    X##_f0 = X##_f0 >> n | X##_f1 << (_FP_W_TYPE_SIZE - n - 1) << 1;	\
+    X##_f1 >>= n;							\
   } while (0)
 
 /* Right shift with sticky-lsb.  */
 #define _FP_FRAC_SRS_2(X,N,sz)						\
   do {									\
-    if ((N) < _FP_W_TYPE_SIZE)						\
-      {									\
-	X##_f0 = (X##_f1 << (_FP_W_TYPE_SIZE - (N)) | X##_f0 >> (N) |	\
-		  (__builtin_constant_p(N) && (N) == 1			\
-		   ? X##_f0 & 1						\
-		   : (X##_f0 << (_FP_W_TYPE_SIZE - (N))) != 0));	\
-	X##_f1 >>= (N);							\
-      }									\
-    else								\
+    int n = (N);							\
+    if (n >= _FP_W_TYPE_SIZE)						\
       {									\
-	X##_f0 = (X##_f1 >> ((N) - _FP_W_TYPE_SIZE) |			\
-	          (((X##_f1 << (2 * _FP_W_TYPE_SIZE - (N))) |		\
-		   X##_f0) != 0));					\
+	X##_f0 = X##_f1;						\
 	X##_f1 = 0;							\
+	n -= _FP_W_TYPE_SIZE;						\
       }									\
+    X##_f0 = (X##_f1 << (_FP_W_TYPE_SIZE - n - 1) << 1 | X##_f0 >> n |	\
+	      ((X##_f0 << (_FP_W_TYPE_SIZE - n - 1) << 1) != 0));	\
+    X##_f1 >>= n;							\
   } while (0)
 
 #define _FP_FRAC_ADDI_2(X,I) \

@@ -398,20 +383,18 @@
 
 #define _FP_FRAC_ASSEMBLE_2(r, X, rsize)	\
   do {						\
-    if (rsize <= _FP_W_TYPE_SIZE)		\
-      r = X##_f0;				\
-    else					\
-      {						\
-	r = X##_f1;				\
-	r <<= _FP_W_TYPE_SIZE;			\
-	r += X##_f0;				\
-      }						\
+    r = X##_f1;					\
+    r <<= _FP_W_TYPE_SIZE - 1;			\
+    r <<= 1;					\
+    r += X##_f0;				\
   } while (0)
 
-#define _FP_FRAC_DISASSEMBLE_2(X, r, rsize)				\
-  do {									\
-    X##_f0 = r;								\
-    X##_f1 = (rsize <= _FP_W_TYPE_SIZE ? 0 : r >> _FP_W_TYPE_SIZE);	\
+#define _FP_FRAC_DISASSEMBLE_2(X, r, rsize)	\
+  do {						\
+    X##_f0 = r;					\
+    r >>= _FP_W_TYPE_SIZE - 1;			\
+    r >>= 1;					\
+    X##_f1 = r;					\
   } while (0)
 
 /*

-- 
1.5.3.4.208.g805a

On Fri, 2008-02-22 at 21:01 +0100, Segher Boessenkool wrote:

Signed-off-by: Segher Boessenkool <redacted>
---

Amen !

_However_ there are significant code changes in there, and I don't
actually understand that code (well, I admit I haven't tried),
so it could definitely use a bit of a commit message explaining
the rationale (you are removing a lot of stuff), and maybe somebody
can run a few tests to make sure things work fine ?

Ben.

_However_ there are significant code changes in there, and I don't
actually understand that code (well, I admit I haven't tried),

Yeah, it's written in 70's style C.  Yuck.

so it could definitely use a bit of a commit message explaining
the rationale

Right.  I had to fix git-send-email and then I forgot to type up
some more comments.

(you are removing a lot of stuff),

Not actually, more below.

and maybe somebody
can run a few tests to make sure things work fine ?

That would be nice.  I don't know any comprehensive IEEE FP test suite
to use on this, nor do I have a platform that normally uses this code
(though I bet I could force a 750 to use it, some way).

I'll resend with some coherent checkin comment after someone has tested
this :-)


This patch is a prime example why diff -c is so much more readable
than diff -u.  But let's not digress, let's look at the code!

So the code used to look like:


#define _FP_FRAC_SLL_2(X,N)                                             
\
   do {                                                                  
\
     if ((N) < _FP_W_TYPE_SIZE)                                          
\
       {                                                                 
\
         if (__builtin_constant_p(N) && (N) == 1)                        
\
           {                                                             
\
             X##_f1 = X##_f1 + X##_f1 + (((_FP_WS_TYPE)(X##_f0)) < 0);   
\
             X##_f0 += X##_f0;                                           
\
           }                                                             
\
         else                                                            
\
           {                                                             
\
             X##_f1 = X##_f1 << (N) | X##_f0 >> (_FP_W_TYPE_SIZE - (N)); 
\
             X##_f0 <<= (N);                                             
\
           }                                                             
\
       }                                                                 
\
     else                                                                
\
       {                                                                 
\
         X##_f1 = X##_f0 << ((N) - _FP_W_TYPE_SIZE);                     
\
         X##_f0 = 0;                                                     
\
       }                                                                 
\
   } while (0)


and after my change it is:


#define _FP_FRAC_SLL_2(X,N)                                             
\
   do {                                                                  
\
     int n = (N);                                                        
\
     if (n >= _FP_W_TYPE_SIZE)                                           
\
       {                                                                 
\
         X##_f1 = X##_f0;                                                
\
         X##_f0 = 0;                                                     
\
         n -= _FP_W_TYPE_SIZE;                                           
\
       }                                                                 
\
     X##_f1 = X##_f1 << n | X##_f0 >> (_FP_W_TYPE_SIZE - n - 1) >> 1;    
\
     X##_f0 <<= n;                                                       
\
   } while (0)


The __builtin_constant_p(N) && (N == 1) special casing in the original
is just noise, it won't result in more efficient code.  When N is a
compile-time constant (remember, this "function" is a preprocessor 
macro),
one of the two branches of the "if" in the original evokes undefined
behaviour (shift by a negative number, resp. shift by a number >= 32).
I rewrote this to "shift" by a whole word first if necessary, and then
by whatever is left.


With recent GCC, all this nonsense doesn't help a bit: f could just have
been a u64, with no worse code generated.  OTOH, I don't really feel
like rewriting all of this.  I might have to though, if I want to get 
rid
of all the "might be used uninitialised" warnings and errors as well :-(


Segher