Decoding 32-bit MIPS instructions

Question

For a homework assignment I've been given the task of parsing out information from an 32-bit MIPS instruction. (For more information on the instruction formats, see here). The instructor has provided us with a header file enumerating all of the functions required, and it's my job to implement them.

Currently, this is what the implementation for one of my functions looks like:

void instruction_partition(unsigned instruction, unsigned *op, unsigned *r1, unsigned *r2, unsigned *r3, unsigned *funct, unsigned *offset, unsigned *jsec)
{
 assert(op && r1 && r2 && r3 && funct && offset && jsec);
 *op = (instruction >> 26) & 0x3F;
 *r1 = (instruction >> 21) & 0x1F;
 *r2 = (instruction >> 16) & 0x1F;
 *r3 = (instruction >> 11) & 0x1F;
 *offset = (instruction >> 6) & 0x1F;
 *funct = instruction & 0x3F;
 *jsec = instruction & 0x3FFFFFF;
}

In a scenario such as this, I'd almost be tempted to hard-code the shift values and bitmasks, despite it being ingrained within me to avoid doing so at all costs.

I tried a few other configurations, but neither resonated with me. Any thoughts?

void instruction_partition(unsigned instruction, unsigned *op, unsigned *r1, unsigned *r2, unsigned *r3, unsigned *funct, unsigned *offset, unsigned *jsec)
{
 assert(op && r1 && r2 && r3 && funct && offset && jsec);
 struct { unsigned offset; unsigned bitMask; } 
 opMaskFormat = { 26, 0x3F }, 
 r1MaskFormat = { 21, 0x1F },
 r2MaskFormat = { 16, 0x1F },
 r3MaskFormat = { 11, 0x1F },
 functMaskFormat = { 6, 0x1F },
 offsetMaskFormat = { 0, 0x3F },
 jsecMaskFormat = { 0, 0x3FFFFFF }; 
 *op = (instruction >> opMaskFormat.offset) & opMaskFormat.bitMask;
 *r1 = (instruction >> r1MaskFormat.offset) & opMaskFormat.bitMask;
 *r2 = (instruction >> r2MaskFormat.offset) & r2MaskFormat.bitMask;
 *r3 = (instruction >> r3MaskFormat.offset) & r3MaskFormat.bitMask;
 *offset = (instruction >> offsetMaskFormat.offset) & offsetMaskFormat.bitMask;
 *funct = (instruction >> functMaskFormat.shift) & functMaskFormat.bitMask;
 *jsec = (instruction >> jsecMaskFormat.offset) & jsecMaskFormat.bitMask;
}

void instruction_partition(unsigned instruction, unsigned *op, unsigned *r1, unsigned *r2, unsigned *r3, unsigned *funct, unsigned *offset, unsigned *jsec)
{
 assert(op && r1 && r2 && r3 && funct && offset && jsec);
 unsigned SIX_BIT_MASK = 0x3F, 
 FIVE_BIT_MASK = 0x1F,
 TWENTY_SIX_BIT_MASK = 0x3FFFFFF;
 *op = (instruction >> 26) & SIX_BIT_MASK;
 *r1 = (instruction >> 21) & FIVE_BIT_MASK;
 *r2 = (instruction >> 16) & FIVE_BIT_MASK;
 *r3 = (instruction >> 11) & FIVE_BIT_MASK;
 *offset = (instruction >> 6) & FIVE_BIT_MASK;
 *funct = instruction & SIX_BIT_MASK;
 *jsec = instruction & TWENTY_SIX_BIT_MASK;
}

• phuclv: what's the point of the assert? 0x00000000 (nop) is a valid operation in MIPS
• kylemart: @LưuVĩnhPhúc It still parses 0x000000000. The procedure parses the instruction's fields into memory referenced by the pointers provided to it. The assert is just ensuring that those pointer values aren't NULL. Note that the instruction parameter isn't included in the assert.

Answer 1

Since the instruction is a 32-bit number, you should use uint32_t, rather than assuming that unsigned is 32 bits on the host.

The first version is fine, I think, because it has the least clutter. An experienced programmer will have no problem figuring out what it does. If you are concerned about the mental arithmetic required to figure out the width of the bitmasks, you could write it like this:

void instruction_partition(uint32_t instruction, unsigned *op, unsigned *r1, unsigned *r2, unsigned *r3, unsigned *funct, unsigned *offset, unsigned *jsec)
{
 assert(op && r1 && r2 && r3 && funct && offset && jsec);
 *op = (instruction >> 26) & ((1 << 6) - 1); /* 6 most significant bits */
 *r1 = (instruction >> 21) & ((1 << 5) - 1);
 *r2 = (instruction >> 16) & ((1 << 5) - 1);
 *r3 = (instruction >> 11) & ((1 << 5) - 1);
 *offset = (instruction >> 6) & ((1 << 5) - 1);
 *funct = instruction & ((1 << 6) - 1); /* 6 least significant bits */
 *jsec = instruction & ((1 << 26) - 1);
}

This should compile to identical code as your first implementation. Technically, the opcode doesn't need to be masked:

 *op = instruction >> 26;

• kylemart: Sadly, my instructor has an extremely odd coding style. I can't change the function header (as mentioned in the OP), but I definitely agree with your comment regarding the usage of uint32_t. (In fact, I had thought the exact same thing you said the moment I read the instructor's header file.)
• 200_success: That's too bad. In that case, I recommend writing a static assertion like _Static_assert(sizeof instruction == 4, "Host platform has incompatible integer size");, and perhaps a comment in protest.
• kylemart: If I could upvote that comment, I would. This is the first time I've heard of a compile-time assert. That's so cool! Good advice
• mkrieger1: Isn't x & ((1 << n) - 1) the same as x % (1 << n)? That would reduce some more of the complexity.