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
