Re: [PATCH] [RFC] Proposed changes to eliminate 'union mips_instruction' type.

[David Daney <ddaney.cavm@gmail.com>]

On 01/14/2013 10:13 PM, Steven J. Hill wrote:
> From: "Steven J. Hill" <sjhill@mips.com>
>
> This patch shows the use of macros in place of 'union mips_instruction'
> type.

Why?  What are the benefits of doing this?

> I converted all usages of 'j_format' and 'r_format' to show how
> the code and macros could look and be defined. I have tested these
> changes on big and little endian platforms.
>
> I want input from everyone, please!!! I want consensus on the macro
> definitions, placement of parenthesis for them, spacing in the header
> file, etc. This is your chance to be completely anal and have fun
> arguments over how things should be. I would also like input on how
> the maintainers would like the patchsets to look like. For example:
>
>    [1/X] - Convert 'j_format'
>    [2/X] - Convert 'r_format'
>    [3/X] - Convert 'f_format'
>    [4/X] - Convert 'u_format'
>    ...
>    [X/X] - Remove usage of 'union mips_instruction' type completely.
>
> Also, I noticed 'p_format' is not used anywhere. Can we kill it? Be
> picky and help with this conversion. Thanks.
>
> Signed-off-by: Steven J. Hill <sjhill@mips.com>
> ---
>   arch/mips/include/asm/inst.h   |   66 +++++++++++-----------------------------
>   arch/mips/kernel/branch.c      |   13 ++++----
>   arch/mips/kernel/jump_label.c  |   10 +++---
>   arch/mips/kernel/kgdb.c        |   10 ++----
>   arch/mips/kernel/kprobes.c     |   18 +++++------
>   arch/mips/kernel/process.c     |   10 +++---
>   arch/mips/oprofile/backtrace.c |    2 +-
>   7 files changed, 46 insertions(+), 83 deletions(-)
>
> diff --git a/arch/mips/include/asm/inst.h b/arch/mips/include/asm/inst.h
> index ab84064..856b14e 100644
> --- a/arch/mips/include/asm/inst.h
> +++ b/arch/mips/include/asm/inst.h
> @@ -192,15 +192,27 @@ enum lx_func {
>   	lbx_op	= 0x16,
>   };
>
> +#define INSN_OPCODE(insn)		(insn >> 26)
> +#define INSN_RS(insn)			((insn >> 21) & 0x1f)
> +#define INSN_RT(insn)			((insn >> 16) & 0x1f)
> +#define INSN_RD(insn)			((insn >> 11) & 0x1f)
> +#define INSN_RE(insn)			((insn >> 6) & 0x1f)
> +#define INSN_FUNC(insn)			(insn & 0x0000003f)
> +
> +#define J_INSN(op,target)		((op << 26) | target)

What is the type of J_INSN()?  What happens if target overflows into the 
'op' field?


> +#define J_INSN_TARGET(insn)		(insn & 0x03ffffff)
> +#define R_INSN(op,rs,rt,rd,re,func)	((op << 26) | (rs << 21) |	\
> +					 (rt << 16) | (rd << 11) |	\
> +					 (re << 6) | func)
> +#define F_INSN(op,fmt,rt,rd,re,func)	R_INSN(op,fmt,rt,rd,re,func)
> +#define F_INSN_FMT(insn)		INSN_RS(insn)
> +#define U_INSN(op,rs,uimm)		((op << 26) | (rs << 21) | uimmediate)
[...]
> +	unsigned int n_insn = insn.word;

I don't like that the width of an insn is not obvious by looking at the 
code.

Can we, assuming we merge something like this, make it something like 
u32, or insn_t?  I'm not sure which is better.


[...]

David Daney