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

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

[Steven J. Hill]

From: "Steven J. Hill" <sjhill@mips.com>

This patch shows the use of macros in place of 'union mips_instruction'
type. 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)
+#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)
+#define U_INSN_UIMM(insn)		(insn & 0x0000ffff)
+
 /*
  * Damn ...  bitfields depend from byteorder :-(
  */
 #ifdef __MIPSEB__
-struct j_format {	/* Jump format */
-	unsigned int opcode : 6;
-	unsigned int target : 26;
-};
-
 struct i_format {	/* Immediate format (addi, lw, ...) */
 	unsigned int opcode : 6;
 	unsigned int rs : 5;
@@ -223,24 +235,6 @@ struct c_format {	/* Cache (>= R6000) format */
 	unsigned int simmediate : 16;
 };
 
-struct r_format {	/* Register format */
-	unsigned int opcode : 6;
-	unsigned int rs : 5;
-	unsigned int rt : 5;
-	unsigned int rd : 5;
-	unsigned int re : 5;
-	unsigned int func : 6;
-};
-
-struct p_format {	/* Performance counter format (R10000) */
-	unsigned int opcode : 6;
-	unsigned int rs : 5;
-	unsigned int rt : 5;
-	unsigned int rd : 5;
-	unsigned int re : 5;
-	unsigned int func : 6;
-};
-
 struct f_format {	/* FPU register format */
 	unsigned int opcode : 6;
 	unsigned int : 1;
@@ -268,12 +262,6 @@ struct b_format { /* BREAK and SYSCALL */
 };
 
 #elif defined(__MIPSEL__)
-
-struct j_format {	/* Jump format */
-	unsigned int target : 26;
-	unsigned int opcode : 6;
-};
-
 struct i_format {	/* Immediate format */
 	signed int simmediate : 16;
 	unsigned int rt : 5;
@@ -296,24 +284,6 @@ struct c_format {	/* Cache (>= R6000) format */
 	unsigned int opcode : 6;
 };
 
-struct r_format {	/* Register format */
-	unsigned int func : 6;
-	unsigned int re : 5;
-	unsigned int rd : 5;
-	unsigned int rt : 5;
-	unsigned int rs : 5;
-	unsigned int opcode : 6;
-};
-
-struct p_format {	/* Performance counter format (R10000) */
-	unsigned int func : 6;
-	unsigned int re : 5;
-	unsigned int rd : 5;
-	unsigned int rt : 5;
-	unsigned int rs : 5;
-	unsigned int opcode : 6;
-};
-
 struct f_format {	/* FPU register format */
 	unsigned int func : 6;
 	unsigned int re : 5;
@@ -348,11 +318,9 @@ union mips_instruction {
 	unsigned int word;
 	unsigned short halfword[2];
 	unsigned char byte[4];
-	struct j_format j_format;
 	struct i_format i_format;
 	struct u_format u_format;
 	struct c_format c_format;
-	struct r_format r_format;
 	struct f_format f_format;
 	struct ma_format ma_format;
 	struct b_format b_format;
diff --git a/arch/mips/kernel/branch.c b/arch/mips/kernel/branch.c
index 4d735d0..d51fa79 100644
--- a/arch/mips/kernel/branch.c
+++ b/arch/mips/kernel/branch.c
@@ -34,18 +34,19 @@ int __compute_return_epc_for_insn(struct pt_regs *regs,
 	unsigned int bit, fcr31, dspcontrol;
 	long epc = regs->cp0_epc;
 	int ret = 0;
+	unsigned int n_insn = insn.word;
 
 	switch (insn.i_format.opcode) {
 	/*
-	 * jr and jalr are in r_format format.
+	 * jr and jalr are in return instruction format.
 	 */
 	case spec_op:
-		switch (insn.r_format.func) {
+		switch (INSN_FUNC(n_insn)) {
 		case jalr_op:
-			regs->regs[insn.r_format.rd] = epc + 8;
+			regs->regs[INSN_RD(n_insn)] = epc + 8;
 			/* Fall through */
 		case jr_op:
-			regs->cp0_epc = regs->regs[insn.r_format.rs];
+			regs->cp0_epc = regs->regs[INSN_RS(n_insn)];
 			break;
 		}
 		break;
@@ -119,7 +120,7 @@ int __compute_return_epc_for_insn(struct pt_regs *regs,
 		break;
 
 	/*
-	 * These are unconditional and in j_format.
+	 * These are unconditional and are jump instruction format.
 	 */
 	case jal_op:
 		regs->regs[31] = regs->cp0_epc + 8;
@@ -127,7 +128,7 @@ int __compute_return_epc_for_insn(struct pt_regs *regs,
 		epc += 4;
 		epc >>= 28;
 		epc <<= 28;
-		epc |= (insn.j_format.target << 2);
+		epc |= (J_INSN_TARGET(n_insn) << 2);
 		regs->cp0_epc = epc;
 		break;
 
diff --git a/arch/mips/kernel/jump_label.c b/arch/mips/kernel/jump_label.c
index 6001610..d938642 100644
--- a/arch/mips/kernel/jump_label.c
+++ b/arch/mips/kernel/jump_label.c
@@ -23,9 +23,8 @@
 void arch_jump_label_transform(struct jump_entry *e,
 			       enum jump_label_type type)
 {
-	union mips_instruction insn;
-	union mips_instruction *insn_p =
-		(union mips_instruction *)(unsigned long)e->code;
+	unsigned int insn;
+	unsigned int *insn_p = (unsigned int *)(unsigned long)e->code;
 
 	/* Jump only works within a 256MB aligned region. */
 	BUG_ON((e->target & ~J_RANGE_MASK) != (e->code & ~J_RANGE_MASK));
@@ -34,10 +33,9 @@ void arch_jump_label_transform(struct jump_entry *e,
 	BUG_ON((e->target & 3) != 0);
 
 	if (type == JUMP_LABEL_ENABLE) {
-		insn.j_format.opcode = j_op;
-		insn.j_format.target = (e->target & J_RANGE_MASK) >> 2;
+		insn = J_INSN(j_op, (e->target & J_RANGE_MASK) >> 2);
 	} else {
-		insn.word = 0; /* nop */
+		insn = 0; /* nop */
 	}
 
 	get_online_cpus();
diff --git a/arch/mips/kernel/kgdb.c b/arch/mips/kernel/kgdb.c
index 23817a6..89887c5 100644
--- a/arch/mips/kernel/kgdb.c
+++ b/arch/mips/kernel/kgdb.c
@@ -368,13 +368,9 @@ struct kgdb_arch arch_kgdb_ops;
  */
 int kgdb_arch_init(void)
 {
-	union mips_instruction insn = {
-		.r_format = {
-			.opcode = spec_op,
-			.func   = break_op,
-		}
-	};
-	memcpy(arch_kgdb_ops.gdb_bpt_instr, insn.byte, BREAK_INSTR_SIZE);
+	unsigned int insn = R_INSN(spec_op, 0, 0, 0, 0, break_op);
+
+	memcpy(arch_kgdb_ops.gdb_bpt_instr, &insn, BREAK_INSTR_SIZE);
 
 	register_die_notifier(&kgdb_notifier);
 
diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
index 158467d..bc24241 100644
--- a/arch/mips/kernel/kprobes.c
+++ b/arch/mips/kernel/kprobes.c
@@ -53,16 +53,16 @@ static const union mips_instruction breakpoint2_insn = {
 DEFINE_PER_CPU(struct kprobe *, current_kprobe);
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
-static int __kprobes insn_has_delayslot(union mips_instruction insn)
+static int __kprobes insn_has_delayslot(unsigned int insn)
 {
-	switch (insn.i_format.opcode) {
+	switch (INSN_OPCODE(insn)) {
 
 		/*
 		 * This group contains:
-		 * jr and jalr are in r_format format.
+		 * jr and jalr are in return instruction format.
 		 */
 	case spec_op:
-		switch (insn.r_format.func) {
+		switch (R_INSN_FUNC(insn)) {
 		case jr_op:
 		case jalr_op:
 			break;
@@ -78,7 +78,7 @@ static int __kprobes insn_has_delayslot(union mips_instruction insn)
 	case bcond_op:
 
 		/*
-		 * These are unconditional and in j_format.
+		 * These are unconditional and in jump instruction format.
 		 */
 	case jal_op:
 	case j_op:
@@ -155,7 +155,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 
 	if ((probe_kernel_read(&prev_insn, p->addr - 1,
 				sizeof(mips_instruction)) == 0) &&
-				insn_has_delayslot(prev_insn)) {
+				insn_has_delayslot(prev_insn.word)) {
 		pr_notice("Kprobes for branch delayslot are not supported\n");
 		ret = -EINVAL;
 		goto out;
@@ -181,7 +181,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	 * using a normal breakpoint instruction in the next slot.
 	 * So, read the instruction and save it for later execution.
 	 */
-	if (insn_has_delayslot(insn))
+	if (insn_has_delayslot(insn.word))
 		memcpy(&p->ainsn.insn[0], p->addr + 1, sizeof(kprobe_opcode_t));
 	else
 		memcpy(&p->ainsn.insn[0], p->addr, sizeof(kprobe_opcode_t));
@@ -294,7 +294,7 @@ static void prepare_singlestep(struct kprobe *p, struct pt_regs *regs,
 	if (p->opcode.word == breakpoint_insn.word ||
 	    p->opcode.word == breakpoint2_insn.word)
 		regs->cp0_epc = (unsigned long)p->addr;
-	else if (insn_has_delayslot(p->opcode)) {
+	else if (insn_has_delayslot(((union mips_instruction)p->opcode).word)) {
 		ret = evaluate_branch_instruction(p, regs, kcb);
 		if (ret < 0) {
 			pr_notice("Kprobes: Error in evaluating branch\n");
@@ -320,7 +320,7 @@ static void __kprobes resume_execution(struct kprobe *p,
 				       struct pt_regs *regs,
 				       struct kprobe_ctlblk *kcb)
 {
-	if (insn_has_delayslot(p->opcode))
+	if (insn_has_delayslot(((union mips_instruction)p->opcode).word))
 		regs->cp0_epc = kcb->target_epc;
 	else {
 		unsigned long orig_epc = kcb->kprobe_saved_epc;
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index a11c6f9..e160923 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -248,13 +248,13 @@ static inline int is_ra_save_ins(union mips_instruction *ip)
 		ip->i_format.rt == 31;
 }
 
-static inline int is_jal_jalr_jr_ins(union mips_instruction *ip)
+static inline int is_jal_jalr_jr_ins(unsigned int insn)
 {
-	if (ip->j_format.opcode == jal_op)
+	if (INSN_OPCODE(insn) == jal_op)
 		return 1;
-	if (ip->r_format.opcode != spec_op)
+	if (INSN_OPCODE(insn) != spec_op)
 		return 0;
-	return ip->r_format.func == jalr_op || ip->r_format.func == jr_op;
+	return INSN_FUNC(insn) == jalr_op || INSN_FUNC(insn) == jr_op;
 }
 
 static inline int is_sp_move_ins(union mips_instruction *ip)
@@ -285,7 +285,7 @@ static int get_frame_info(struct mips_frame_info *info)
 
 	for (i = 0; i < max_insns; i++, ip++) {
 
-		if (is_jal_jalr_jr_ins(ip))
+		if (is_jal_jalr_jr_ins(ip->word))
 			break;
 		if (!info->frame_size) {
 			if (is_sp_move_ins(ip))
diff --git a/arch/mips/oprofile/backtrace.c b/arch/mips/oprofile/backtrace.c
index 6854ed5..26d05cd 100644
--- a/arch/mips/oprofile/backtrace.c
+++ b/arch/mips/oprofile/backtrace.c
@@ -52,7 +52,7 @@ static inline int is_sp_move_ins(union mips_instruction *ip)
 static inline int is_end_of_function_marker(union mips_instruction *ip)
 {
 	/* jr ra */
-	if (ip->r_format.func == jr_op && ip->r_format.rs == 31)
+	if (R_INSN_FUNC(ip->word) && R_INSN_RS(ip->word) == 31)
 		return 1;
 	/* lui gp */
 	if (ip->i_format.opcode == lui_op && ip->i_format.rt == 28)
-- 
1.7.9.5

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

[Ralf Baechle]

On Tue, Jan 15, 2013 at 12:13:40AM -0600, 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. 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.

Mayne not kill it completely abut leave a comment mentioning its existence
for the sake of completeness.

  Ralf

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

[David Daney]

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

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

[Hill, Steven]

>> This patch shows the use of macros in place of 'union mips_instruction'
>> type.
>>
> Why?  What are the benefits of doing this?
>
The microMIPS patches will not make it in due to the 4x size increase of this structure. Also, as was mentioned on the list previously by Ralf, it should have been done like this years back.

>> +
>> +#define J_INSN(op,target)            ((op << 26) | target)
>
> What is the type of J_INSN()?  What happens if target overflows into the
> 'op' field?
>
Jump instruction, which is evident from the code removed in the patch. The macros are not done, this is a prototype and bounds checking will of course be done for the final. I mostly wanted to see if people were happy with the macro names, how they are laid out in the header file and syntactical nits.

>> +#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.
>
I was planning on making it a 'u32' but I am open to either one. Ralf, which would you prefer?

-Steve

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

[David Daney]

On 01/15/2013 02:19 PM, Hill, Steven wrote:
>>> This patch shows the use of macros in place of 'union mips_instruction'
>>> type.
>>>
>> Why?  What are the benefits of doing this?
>>
> The microMIPS patches will not make it in due to the 4x size increase of this structure. Also, as was mentioned on the list previously by Ralf, it should have been done like this years back.

A matter of opinion.  Bitfields have a certain elegance

Personally I would investigate machine generating the file with the 
structure definitions.  That way you could insure consistency between 
big and little endian versions.


>
>>> +
>>> +#define J_INSN(op,target)            ((op << 26) | target)
>>
>> What is the type of J_INSN()?  What happens if target overflows into the
>> 'op' field?
>>
> Jump instruction, which is evident from the code removed in the patch. The macros are not done, this is a prototype and bounds checking will of course be done for the final. I mostly wanted to see if people were happy with the macro names, how they are laid out in the header file and syntactical nits.
>

For me it is much more important that the data types be correct and the 
overflow conditions are handled (and perhaps also warned about).

The order in the file I don't care about.

>>> +#define J_INSN_TARGET(insn)          (insn & 0x03ffffff)

INSN_J_TARGET ...

>>> +#define R_INSN(op,rs,rt,rd,re,func)  ((op << 26) | (rs << 21) |      \
>>> +                                      (rt << 16) | (rd << 11) |      \
>>> +                                      (re << 6) | func)


#define INSN_RANGE_CHECK(v, bits) ({ \
     u32 val = (v); \
     u32 mask = (1 << bits) - 1; \
     WARN((v & mask) != v, "YOU LOSE"); \
     val; \
})

#define INSN_TYPE_R(op, rs, rt, rd, re, func) \
  ((INSN_RANGE_CHECK((op), 6) << 26 | \
   (INSN_RANGE_CHECK((rs), 5) << 21 | \
   (INSN_RANGE_CHECK((rt), 5) << 16 | \
   (INSN_RANGE_CHECK((rd), 5) << 11 | \
   (INSN_RANGE_CHECK((re), 5) << 6 | \
   (INSN_RANGE_CHECK((func), 6))

But you cannot use that as a static initializer.


>>> +#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.
>>
> I was planning on making it a 'u32' but I am open to either one. Ralf, which would you prefer?
>
> -Steve
>

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

[Ralf Baechle]

On Tue, Jan 15, 2013 at 02:39:15PM -0800, David Daney wrote:

So this should be fairly readable, far less code and in especially no
more variants for endianess except a single simple macro.

What do you think?

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

 arch/mips/include/asm/inst.h | 227 +++++++++++++++++--------------------------
 1 file changed, 87 insertions(+), 140 deletions(-)

diff --git a/arch/mips/include/asm/inst.h b/arch/mips/include/asm/inst.h
index ab84064..442e7a4 100644
--- a/arch/mips/include/asm/inst.h
+++ b/arch/mips/include/asm/inst.h
@@ -196,154 +196,100 @@ enum lx_func {
  * Damn ...  bitfields depend from byteorder :-(
  */
 #ifdef __MIPSEB__
-struct j_format {	/* Jump format */
-	unsigned int opcode : 6;
-	unsigned int target : 26;
-};
-
-struct i_format {	/* Immediate format (addi, lw, ...) */
-	unsigned int opcode : 6;
-	unsigned int rs : 5;
-	unsigned int rt : 5;
-	signed int simmediate : 16;
-};
-
-struct u_format {	/* Unsigned immediate format (ori, xori, ...) */
-	unsigned int opcode : 6;
-	unsigned int rs : 5;
-	unsigned int rt : 5;
-	unsigned int uimmediate : 16;
-};
-
-struct c_format {	/* Cache (>= R6000) format */
-	unsigned int opcode : 6;
-	unsigned int rs : 5;
-	unsigned int c_op : 3;
-	unsigned int cache : 2;
-	unsigned int simmediate : 16;
-};
-
-struct r_format {	/* Register format */
-	unsigned int opcode : 6;
-	unsigned int rs : 5;
-	unsigned int rt : 5;
-	unsigned int rd : 5;
-	unsigned int re : 5;
-	unsigned int func : 6;
-};
-
-struct p_format {	/* Performance counter format (R10000) */
-	unsigned int opcode : 6;
-	unsigned int rs : 5;
-	unsigned int rt : 5;
-	unsigned int rd : 5;
-	unsigned int re : 5;
-	unsigned int func : 6;
-};
-
-struct f_format {	/* FPU register format */
-	unsigned int opcode : 6;
-	unsigned int : 1;
-	unsigned int fmt : 4;
-	unsigned int rt : 5;
-	unsigned int rd : 5;
-	unsigned int re : 5;
-	unsigned int func : 6;
-};
-
-struct ma_format {	/* FPU multiply and add format (MIPS IV) */
-	unsigned int opcode : 6;
-	unsigned int fr : 5;
-	unsigned int ft : 5;
-	unsigned int fs : 5;
-	unsigned int fd : 5;
-	unsigned int func : 4;
-	unsigned int fmt : 2;
-};
-
-struct b_format { /* BREAK and SYSCALL */
-	unsigned int opcode:6;
-	unsigned int code:20;
-	unsigned int func:6;
-};
+#define BITFIELD_FIELD(field, more)					\
+	field;								\
+	more
 
 #elif defined(__MIPSEL__)
 
-struct j_format {	/* Jump format */
-	unsigned int target : 26;
-	unsigned int opcode : 6;
-};
-
-struct i_format {	/* Immediate format */
-	signed int simmediate : 16;
-	unsigned int rt : 5;
-	unsigned int rs : 5;
-	unsigned int opcode : 6;
-};
-
-struct u_format {	/* Unsigned immediate format */
-	unsigned int uimmediate : 16;
-	unsigned int rt : 5;
-	unsigned int rs : 5;
-	unsigned int opcode : 6;
-};
-
-struct c_format {	/* Cache (>= R6000) format */
-	unsigned int simmediate : 16;
-	unsigned int cache : 2;
-	unsigned int c_op : 3;
-	unsigned int rs : 5;
-	unsigned int opcode : 6;
-};
-
-struct r_format {	/* Register format */
-	unsigned int func : 6;
-	unsigned int re : 5;
-	unsigned int rd : 5;
-	unsigned int rt : 5;
-	unsigned int rs : 5;
-	unsigned int opcode : 6;
-};
-
-struct p_format {	/* Performance counter format (R10000) */
-	unsigned int func : 6;
-	unsigned int re : 5;
-	unsigned int rd : 5;
-	unsigned int rt : 5;
-	unsigned int rs : 5;
-	unsigned int opcode : 6;
-};
-
-struct f_format {	/* FPU register format */
-	unsigned int func : 6;
-	unsigned int re : 5;
-	unsigned int rd : 5;
-	unsigned int rt : 5;
-	unsigned int fmt : 4;
-	unsigned int : 1;
-	unsigned int opcode : 6;
-};
-
-struct ma_format {	/* FPU multiply and add format (MIPS IV) */
-	unsigned int fmt : 2;
-	unsigned int func : 4;
-	unsigned int fd : 5;
-	unsigned int fs : 5;
-	unsigned int ft : 5;
-	unsigned int fr : 5;
-	unsigned int opcode : 6;
-};
-
-struct b_format { /* BREAK and SYSCALL */
-	unsigned int func:6;
-	unsigned int code:20;
-	unsigned int opcode:6;
-};
+#define BITFIELD_FIELD(field, more)					\
+	more								\
+	field;
 
 #else /* !defined (__MIPSEB__) && !defined (__MIPSEL__) */
 #error "MIPS but neither __MIPSEL__ nor __MIPSEB__?"
 #endif
 
+struct j_format {
+	BITFIELD_FIELD(unsigned int opcode : 6,	/* Jump format */
+	BITFIELD_FIELD(unsigned int target : 26,
+	))
+};
+
+struct i_format {			/* signed immediate format */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int rs : 5,
+	BITFIELD_FIELD(unsigned int rt : 5,
+	BITFIELD_FIELD(signed int simmediate : 16,
+	))))
+};
+
+struct u_format {			/* unsigned immediate format */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int rs : 5,
+	BITFIELD_FIELD(unsigned int rt : 5,
+	BITFIELD_FIELD(unsigned int uimmediate : 16,
+	))))
+};
+
+struct c_format {			/* Cache (>= R6000) format */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int rs : 5,
+	BITFIELD_FIELD(unsigned int c_op : 3,
+	BITFIELD_FIELD(unsigned int cache : 2,
+	BITFIELD_FIELD(unsigned int simmediate : 16,
+	)))))
+};
+
+struct r_format {			/* Register format */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int rs : 5,
+	BITFIELD_FIELD(unsigned int rt : 5,
+	BITFIELD_FIELD(unsigned int rd : 5,
+	BITFIELD_FIELD(unsigned int re : 5,
+	BITFIELD_FIELD(unsigned int func : 6,
+	))))))
+};
+
+struct p_format {		/* Performance counter format (R10000) */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int rs : 5,
+	BITFIELD_FIELD(unsigned int rt : 5,
+	BITFIELD_FIELD(unsigned int rd : 5,
+	BITFIELD_FIELD(unsigned int re : 5,
+	BITFIELD_FIELD(unsigned int func : 6,
+	))))))
+};
+
+struct f_format { 			/* FPU register format */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int : 1,
+	BITFIELD_FIELD(unsigned int fmt : 4,
+	BITFIELD_FIELD(unsigned int rt : 5,
+	BITFIELD_FIELD(unsigned int rd : 5,
+	BITFIELD_FIELD(unsigned int re : 5,
+	BITFIELD_FIELD(unsigned int func : 6,
+	)))))))
+};
+
+struct ma_format {		/* FPU multiply and add format (MIPS IV) */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int fr : 5,
+	BITFIELD_FIELD(unsigned int ft : 5,
+	BITFIELD_FIELD(unsigned int fs : 5,
+	BITFIELD_FIELD(unsigned int fd : 5,
+	BITFIELD_FIELD(unsigned int func : 4,
+	BITFIELD_FIELD(unsigned int fmt : 2,
+	)))))))
+};
+
+struct b_format {			/* BREAK and SYSCALL */
+	BITFIELD_FIELD(unsigned int opcode : 6,
+	BITFIELD_FIELD(unsigned int code : 20,
+	BITFIELD_FIELD(unsigned int func : 6,
+	)))
+};
+
 union mips_instruction {
 	unsigned int word;
 	unsigned short halfword[2];
@@ -353,6 +299,7 @@ union mips_instruction {
 	struct u_format u_format;
 	struct c_format c_format;
 	struct r_format r_format;
+	struct p_format p_format;
 	struct f_format f_format;
 	struct ma_format ma_format;
 	struct b_format b_format;

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

[David Daney]

On 01/15/2013 02:39 PM, David Daney wrote:
> On 01/15/2013 02:19 PM, Hill, Steven wrote:
>>>> This patch shows the use of macros in place of 'union mips_instruction'
>>>> type.
>>>>
>>> Why?  What are the benefits of doing this?
>>>
>> The microMIPS patches will not make it in due to the 4x size increase
>> of this structure. Also, as was mentioned on the list previously by
>> Ralf, it should have been done like this years back.
>
> A matter of opinion.  Bitfields have a certain elegance
>
> Personally I would investigate machine generating the file with the
> structure definitions.  That way you could insure consistency between
> big and little endian versions.

Like this:

------------------8<------------------
#include <stdio.h>

struct mips_insn_field {
	const char *field_name;
	const char *field_type;
	int width;
};

struct mips_insn_format {
	const char *format_name;
	const char *comment;
	struct mips_insn_field *fields;
};


static struct mips_insn_format *insn_types[] = {
	&(struct mips_insn_format){"j_format",
				   "Jump format",
				   (struct mips_insn_field[]){{"opcode", "unsigned", 6},
							      {"target", "unsigned", 26},
							      {0}}},

	&(struct mips_insn_format){"i_format",
				   "Immediate format (addi, lw, ...)",
				   (struct mips_insn_field[]){{"opcode", "unsigned", 6},
							      {"rs", "unsigned", 5},
							      {"rt", "unsigned", 5},
							      {"simmediate", "signed", 5},
							      {0}}},

	&(struct mips_insn_format){"u_format",
				   "Unsigned immediate format (ori, xori, ...)",
				   (struct mips_insn_field[]){{"opcode", "unsigned", 6},
							      {"rs", "unsigned", 5},
							      {"rt", "unsigned", 5},
							      {"uimmediate", "unsigned", 5},
							      {0}}},

	&(struct mips_insn_format){"c_format",
				   "Cache (>= R6000) format",
				   (struct mips_insn_field[]){{"opcode", "unsigned", 6},
							      {"rs", "unsigned", 5},
							      {"c_op", "unsigned", 3},
							      {"cacge", "unsigned", 2},
							      {"simmediate", "signed", 5},
							      {0}}},
	NULL
};


static void print_one_field(struct mips_insn_field *field)
{
	printf("\t%s int %s:%d\n", field->field_type, field->field_name, 
field->width);
}

static void print_one_format(struct mips_insn_format *type)
{
	int i;
	int num_fields = 0;
	struct mips_insn_field *field;

	field = type->fields;
	while (field->field_name) {
		num_fields++;
		field++;
	}

	printf("struct %s {", type->format_name);
	if (type->comment)
		printf("\t/* %s */\n", type->comment);
	else
		printf("\n");
	printf("#ifdef __MIPSEB__\n");

	for (i = 0; i < num_fields; i++)
		print_one_field(&type->fields[i]);
		
	printf("#else\n");

	for (i = num_fields - 1; i >= 0; i--)
		print_one_field(&type->fields[i]);

	printf("#endif\n");
	printf("};\n");
}

int main(int argc, char *argv[])
{
	struct mips_insn_format **type = insn_types;

	printf("/* Machine generated, do not edit. */\n");
	printf("/* Edit gen_mips_insns.c instead. */\n");

	while (*type) {
		print_one_format(*type);
		type++;
	}

	return 0;
}

------------------8<------------------


>
>
>>
>>>> +
>>>> +#define J_INSN(op,target)            ((op << 26) | target)
>>>
>>> What is the type of J_INSN()?  What happens if target overflows into the
>>> 'op' field?
>>>
>> Jump instruction, which is evident from the code removed in the patch.
>> The macros are not done, this is a prototype and bounds checking will
>> of course be done for the final. I mostly wanted to see if people were
>> happy with the macro names, how they are laid out in the header file
>> and syntactical nits.
>>
>
> For me it is much more important that the data types be correct and the
> overflow conditions are handled (and perhaps also warned about).
>
> The order in the file I don't care about.
>
>>>> +#define J_INSN_TARGET(insn)          (insn & 0x03ffffff)
>
> INSN_J_TARGET ...
>
>>>> +#define R_INSN(op,rs,rt,rd,re,func)  ((op << 26) | (rs << 21) |      \
>>>> +                                      (rt << 16) | (rd << 11) |      \
>>>> +                                      (re << 6) | func)
>
>
> #define INSN_RANGE_CHECK(v, bits) ({ \
>      u32 val = (v); \
>      u32 mask = (1 << bits) - 1; \
>      WARN((v & mask) != v, "YOU LOSE"); \
>      val; \
> })
>
> #define INSN_TYPE_R(op, rs, rt, rd, re, func) \
>   ((INSN_RANGE_CHECK((op), 6) << 26 | \
>    (INSN_RANGE_CHECK((rs), 5) << 21 | \
>    (INSN_RANGE_CHECK((rt), 5) << 16 | \
>    (INSN_RANGE_CHECK((rd), 5) << 11 | \
>    (INSN_RANGE_CHECK((re), 5) << 6 | \
>    (INSN_RANGE_CHECK((func), 6))
>
> But you cannot use that as a static initializer.
>
>
>>>> +#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.
>>>
>> I was planning on making it a 'u32' but I am open to either one. Ralf,
>> which would you prefer?
>>
>> -Steve
>>
>

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

[Hill, Steven]

I will take whatever method everyone chooses. This is the path I need for microMIPS, so I am an agnostic.

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

[David Daney]

On 01/16/2013 06:16 AM, Ralf Baechle wrote:
> On Tue, Jan 15, 2013 at 02:39:15PM -0800, David Daney wrote:
>
> So this should be fairly readable, far less code and in especially no
> more variants for endianess except a single simple macro.
>
> What do you think?

Very tricky.  I like it.

However, a small change is needed...

[...]
> +#define BITFIELD_FIELD(field, more)					\
> +	field;								\
> +	more
>
>   #elif defined(__MIPSEL__)
>
[...]
> +#define BITFIELD_FIELD(field, more)					\
> +	more								\
> +	field;
>
>   #else /* !defined (__MIPSEB__) && !defined (__MIPSEL__) */
>   #error "MIPS but neither __MIPSEL__ nor __MIPSEB__?"
>   #endif
>
> +struct j_format {
> +	BITFIELD_FIELD(unsigned int opcode : 6,	/* Jump format */
> +	BITFIELD_FIELD(unsigned int target : 26,

... In the very last BITFIELD_FIELD(), you need a valid token as the 
second parameter, otherwise (according to Pinski) C90 behavior is undefined.

Use a ';'



> +	))
> +};
> +
> +struct i_format {			/* signed immediate format */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int rs : 5,
> +	BITFIELD_FIELD(unsigned int rt : 5,
> +	BITFIELD_FIELD(signed int simmediate : 16,
> +	))))
> +};
> +
> +struct u_format {			/* unsigned immediate format */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int rs : 5,
> +	BITFIELD_FIELD(unsigned int rt : 5,
> +	BITFIELD_FIELD(unsigned int uimmediate : 16,
> +	))))
> +};
> +
> +struct c_format {			/* Cache (>= R6000) format */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int rs : 5,
> +	BITFIELD_FIELD(unsigned int c_op : 3,
> +	BITFIELD_FIELD(unsigned int cache : 2,
> +	BITFIELD_FIELD(unsigned int simmediate : 16,
> +	)))))
> +};
> +
> +struct r_format {			/* Register format */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int rs : 5,
> +	BITFIELD_FIELD(unsigned int rt : 5,
> +	BITFIELD_FIELD(unsigned int rd : 5,
> +	BITFIELD_FIELD(unsigned int re : 5,
> +	BITFIELD_FIELD(unsigned int func : 6,
> +	))))))
> +};
> +
> +struct p_format {		/* Performance counter format (R10000) */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int rs : 5,
> +	BITFIELD_FIELD(unsigned int rt : 5,
> +	BITFIELD_FIELD(unsigned int rd : 5,
> +	BITFIELD_FIELD(unsigned int re : 5,
> +	BITFIELD_FIELD(unsigned int func : 6,
> +	))))))
> +};BITFIELD_FIELD(
> +
> +struct f_format { 			/* FPU register format */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int : 1,
> +	BITFIELD_FIELD(unsigned int fmt : 4,
> +	BITFIELD_FIELD(unsigned int rt : 5,
> +	BITFIELD_FIELD(unsigned int rd : 5,
> +	BITFIELD_FIELD(unsigned int re : 5,
> +	BITFIELD_FIELD(unsigned int func : 6,
> +	)))))))
> +};
> +
> +struct ma_format {		/* FPU multiply and add format (MIPS IV) */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int fr : 5,
> +	BITFIELD_FIELD(unsigned int ft : 5,
> +	BITFIELD_FIELD(unsigned int fs : 5,
> +	BITFIELD_FIELD(unsigned int fd : 5,
> +	BITFIELD_FIELD(unsigned int func : 4,
> +	BITFIELD_FIELD(unsigned int fmt : 2,
> +	)))))))
> +};
> +
> +struct b_format {			/* BREAK and SYSCALL */
> +	BITFIELD_FIELD(unsigned int opcode : 6,
> +	BITFIELD_FIELD(unsigned int code : 20,
> +	BITFIELD_FIELD(unsigned int func : 6,
> +	)))
> +};
> +
>   union mips_instruction {
>   	unsigned int word;
>   	unsigned short halfword[2];
> @@ -353,6 +299,7 @@ union mips_instruction {
>   	struct u_format u_format;
>   	struct c_format c_format;
>   	struct r_format r_format;
> +	struct p_format p_format;
>   	struct f_format f_format;
>   	struct ma_format ma_format;
>   	struct b_format b_format;
>
>

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

[Ralf Baechle]

On Wed, Jan 16, 2013 at 02:24:26PM -0800, David Daney wrote:

> ... In the very last BITFIELD_FIELD(), you need a valid token as the
> second parameter, otherwise (according to Pinski) C90 behavior is
> undefined.
> 
> Use a ';'

While Andrew is correct, I don't think this argument matters unless
we're going to export <asm/inst.h> to userspace.  Should we?  Historically
it was meant to be exported and accessed by application code from
<sys/inst.h>.

  Ralf