Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable by ctc1

["Kevin D. Kissell" <kevink@paralogos.com>]

Shane McDonald wrote:
> On Thu, May 6, 2010 at 9:46 AM, Kevin D. Kissell <kevink@paralogos.com> 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.