Re: [Fwd: Re: [PATCH] MIPS32 DSP instructions again]

[Eric Christopher <echristo at redhat dot com>]

On Mon, 2005-06-13 at 15:05 -0700, Chao-ying Fu wrote:
> Hello,
> 
> 1. I think I put extra tab, so that the table is aligned
> for all cases.
> 

It didn't look like it in the diff. Make sure.

> 2. Someone complained about that I put some magic number
> inside my code.  So, I generated the magic number using
> the existing mask.  I can change it back to use the number
> (ex: 64) directly.
> 

Right. You're doing foo + 64 < 127, why not just put in something like:

min_range:
max_range:

and check to make sure that the operand is in the range?

All else fails comment.

> 3. I reuse the 'c' format, so that I don't need to create
> a new format for the same shift and the same mask and the
> same immediate.  I can create a new mask and the new shift,
> if you want.
> 

This is fine. Comment your code. "This mask is used by "foo" and "blah"
types of instructions"

> 4. Some DSP instructions write or read accumulators which
> may be $ac0, $ac1, $ac2, $ac3.  Because $ac0 is HI/LO,
> these DSP instructions need the attritubes (WR_HILO/RD_HILO)
> to inform the assembler to maintain correct HI/LO dependences.
> Or, if the dependences of accumulators and HI/LO are not necessary
> at all, I can remove all WR_a and RD_a.

You really really need to document this. Since the instruction set has
no released documentation I have to take your word on how this works.
I'm not happy with this as I've mentioned before.

So, if I've got this correct:

a) The accumulators are:

HI (acc0)
LO (acc1)
new register (acc2)
new register (acc3)

or

b)

HI/LO (acc0)
new register (acc1)
new register (acc2)
new register (acc3)

I'll assume that these new registers are the same size as the HI + LO
register size? 

You need to also comment the patch far more than you have.

-eric