Page MenuHomePhabricator

[mips][microMIPS] Implement disassembler support for 16-bit instructions
ClosedPublic

Authored by jkolek on Nov 6 2014, 8:25 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jkolek updated this revision to Diff 15872.Nov 6 2014, 8:25 AM
jkolek retitled this revision from to [mips][microMIPS] Implement disassembler support for 16-bit instructions.
jkolek updated this object.
jkolek edited the test plan for this revision. (Show Details)
jkolek added reviewers: dsanders, sstankovic, vmedic.
jkolek added a subscriber: zoran.jovanovic.
sstankovic added inline comments.Nov 17 2014, 9:51 AM
lib/Target/Mips/Disassembler/MipsDisassembler.cpp
712 ↗(On Diff #15872)

Align the comment.

731 ↗(On Diff #15872)

This comment is not valid anymore. In this patch you don't use readInstruction16 for mips32r2 (so mips32r2: line is unnecessary). You use it to read 16 bits of a microMIPS instruction (either 16-bit or 32-bit instructions). I think the more appropriate place for the comment is the function readInstruction32, and instead of mips32r2 and microMIPS little-endian byte ordering, the comment should specify the big and little-endian byte ordering of a 32-bit microMIPS instruction. For example:

Big-endian: 0 | 1 | 2 | 3
Little-endian: 1 | 0 | 3 | 2

It is obvious from the ordering, but you may also mention it explicitly that high 16 bits of a 32-bit microMIPS instruction (where the opcode is) always precede the low 16 bits in the instruction stream (that is, they are placed at lower addresses in the instruction stream).

928 ↗(On Diff #15872)

Why not use Mips::GPRMM16RegClassID?

jkolek added a subscriber: Unknown Object (MLST).Nov 18 2014, 5:42 AM
jkolek added inline comments.Nov 21 2014, 7:01 AM
lib/Target/Mips/Disassembler/MipsDisassembler.cpp
928 ↗(On Diff #15872)

Because we are actually using register indexes that doesn't fit into GPRMM16 set.

jkolek updated this revision to Diff 16491.Nov 21 2014, 7:13 AM

Rebased disassembler support for microMIPS 16-bit instructions.

jkolek updated this revision to Diff 16495.Nov 21 2014, 8:48 AM

Function DecodeGPRMM16RegisterClass is reimplemented to use GPRMM16RegClassID instead of GPR32RegClassID.

sstankovic edited edge metadata.Nov 24 2014, 4:30 AM

LGTM, with 3 changes.

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
708 ↗(On Diff #16495)

Add '.' at the end.

711 ↗(On Diff #16495)

You can place all parameters on 3 lines:

static DecodeStatus readInstruction16(ArrayRef<uint8_t> Bytes, uint64_t Address,

uint64_t &Size, uint32_t &Insn,
bool IsBigEndian) {
744 ↗(On Diff #16495)

microMIPS

sstankovic accepted this revision.Nov 24 2014, 4:30 AM
sstankovic edited edge metadata.
This revision is now accepted and ready to land.Nov 24 2014, 4:30 AM
jkolek closed this revision.Nov 24 2014, 5:30 AM
jkolek updated this revision to Diff 16555.

Closed by commit rL222648 (authored by @jkolek).