Shane McDonald wrote:
On Thu, May 6, 2010 at 9:46 AM, Kevin D. Kissell <kevink@xxxxxxxxxxxxx> wrote:
Sergei Shtylyov wrote:
Kevin D. Kissell wrote:
I'm cool with the patch as is, but in the general spirit of regarding
numeric constants other than 0 and 1 as instruments of Satan, it
would probably be even better if those reserved bits were defined
(FPU_CSR_RSVD, or whatever is compatible with existing convention for
such bits) along with the other FCSR bit masks in mipsregs.h, so that
the assigment looks like:
ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) |
ieee_rm[value & 0x3];
0x3 is still neither 0 nor 1, and so remains an instrument of Satan.
How about #defining it also? :-)
In some software engineering cultures, that would be considered a good
idea or even mandatory. This being the Linux kernel, I think it's OK,
because, as Shane remarked, it's a mask that's local to the module and
whose value is "obvious", and such things are pretty commonly handled as
numeric constants in the kernel.
There actually *is* a #define for that field, FPU_CSR_RD, which could be
used here in place of the 0x3, but I'm a little torn about its use. On
one hand "value & ~(FPU_CSR_RSVD | FPU_CSR_RD)" is more clear about what
we're doing, but on the other hand, it's less obvious that "value &
FPU_CSR_RD" is generating an integer in the range 0-3 for an index. So
I'm absolutely fine with the code as written, but wouldn't complain if
someone wanted to make it use the symbolic constant.
I'm torn here, which is why I hadn't changed that at all. I'd rather not
use FPU_CSR_RD, because that defines a value in the rounding mode
bits (which happens to have all the bits set), rather than a mask for the
bits. I'd prefer to define a new constant FPU_CSR_RM with the
value 0x00000003 -- a better name might be FPU_CSR_RM_MASK,
but that's inconsistent with the names of the other FCSR fields.
And, as Kevin said, it doesn't make it clear that we're trying to generate
an index in the range of 0 - 3. I guess we could also define a constant
for the number of bits to shift to get the RM bits into the low order bits,
something like
#define FPU_CSR_RM_SHIFT 0
at which point our code could look like
ctx->fcr31 = (value & ~(FPU_CSR_RSVD | FPU_CSR_RM)) |
ieee_rm[(value & FPU_CSR_RM) >> FPU_CSR_RM_SHIFT];
I'm a little wary of adding the FPU_CSR_RM_SHIFT, because there aren't
any other SHIFT defines for the FCSR, and because the shift value is 0.
But, the above code doesn't look as bad as I originally thought it would,
and it probably is clearer.
Comments?
I agree with Ralf's suggestion to make the further cleanup
a separate patch. Since there isn't a precedent for having
shift counts for the FCSR fields, another option would be
to define a macro, let's call it modeindex() for the sake of
the argument, that's defined wherever ieee_rm is declared,
and which converts an FCSR value into the index:
#define modeindex(v) ((v) & FPU_CSR_RM)
Then the actual code in cop1emu.c could look like:
ctx->fcr31 = (value & ~(FPU_CSR_RSVD | FPU_CSR_RM)) |
ieee_rm[modeindex(value)];
Which is pretty clear, has no cursed constants, and doesn't
create a universe of shift counts where the only one we
care about is zero.
There's also use of the 0x3 magic number in a number of other cases
in this file.
If all the others treat it as a mask, then your proposed
FPR_CSR_RM should be fine for them. If they don't...
Regards,
Kevin K.
