* [PATCH] powerpc: Get rid of invalid shifts in math-emu
@ 2008-02-22 20:01 Segher Boessenkool
2008-02-22 22:24 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 3+ messages in thread
From: Segher Boessenkool @ 2008-02-22 20:01 UTC (permalink / raw)
To: linuxppc-dev
Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc: Get rid of invalid shifts in math-emu
2008-02-22 20:01 [PATCH] powerpc: Get rid of invalid shifts in math-emu Segher Boessenkool
@ 2008-02-22 22:24 ` Benjamin Herrenschmidt
2008-02-22 23:13 ` Segher Boessenkool
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Herrenschmidt @ 2008-02-22 22:24 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev
On Fri, 2008-02-22 at 21:01 +0100, Segher Boessenkool wrote:
> Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
> ---
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.
> 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)
>
> /*
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc: Get rid of invalid shifts in math-emu
2008-02-22 22:24 ` Benjamin Herrenschmidt
@ 2008-02-22 23:13 ` Segher Boessenkool
0 siblings, 0 replies; 3+ messages in thread
From: Segher Boessenkool @ 2008-02-22 23:13 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev
> _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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-02-22 23:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-02-22 20:01 [PATCH] powerpc: Get rid of invalid shifts in math-emu Segher Boessenkool
2008-02-22 22:24 ` Benjamin Herrenschmidt
2008-02-22 23:13 ` Segher Boessenkool
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).