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

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

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
>>
>