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