[PATCH] powerpc: Get rid of invalid shifts in math-emu
Segher Boessenkool
segher at kernel.crashing.org
Sat Feb 23 10:13:09 EST 2008
> _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
More information about the Linuxppc-dev
mailing list