The patch implements microMIPSr6 MFC0, MFC1, MFC2, MFHC0, MFHC1, MFHC2, DMFC0, DMFC1 and DMFC2 instructions.
Details
Diff Detail
Event Timeline
LGTM with the missing MFHC1 for the FGR64 case and the indentation nits.
lib/Target/Mips/MicroMips32r6InstrInfo.td | ||
---|---|---|
582–583 | The FPU register width is independent of the CPU register width. You also need the 64-bit FPU case. | |
lib/Target/Mips/MicroMips64r6InstrInfo.td | ||
100–101 | I mentioned above that the FPU register width is independent of the CPU register width but we do guarantee that it's no smaller than the CPU register width so this is ok. | |
test/MC/Mips/micromips32r6/invalid.s | ||
113–116 | Indentation on the '# CHECK'. | |
test/MC/Mips/micromips32r6/valid.s | ||
260–267 | Indentation on the '# encoding'. |
As with http://reviews.llvm.org/D17328, dmfc2 taking a selector is awaiting clarification.
test/MC/Mips/mips64r5/invalid.s | ||
---|---|---|
19 ↗ | (On Diff #48283) | Issue here, dmfc2 does not appear to actually take a selector. I'm waiting on clarification from the documents group. |
Instructions that access coprocessor 2 don't have a selector. The Impl field should be treated as a 5 bit register field for consistency with binutils. LGTM with that change.
Rebased to work with top of the tree.
Added instruction definition for 64-bit version of MFHC1.
Added decoder table for 64-bit FPU version.
Invalid tests for MF* and DMFC* in MIPS32 and MIPS64 moved to another patch (already commited).
Fixed indentations.
LGTM with a nit and missing predicate added.
Added decoder table for 64-bit FPU version.
This hadn't occurred to me at the time but it raises a good point. The existing FP64 disassembly support is currently in the Mips64 decoder table but this means that disassembling Mips32+FP64 objects won't yield the correct MCInstr objects. Our existing disassembler tests won't detect this because the instructions are syntactically identical at the assembly level. This existing bug doesn't need fixing in this patch, I just mention it as something we'll need to address at some point.
lib/Target/Mips/Disassembler/MipsDisassembler.cpp | ||
---|---|---|
922 | You need to check for FP64 too. | |
lib/Target/Mips/MicroMips32r6InstrInfo.td | ||
1055 | The 'FPU' suffix by itself is a little misleading. It's really about 64-bit FPU's (FP64) rather than FPU's in general. |
You need to check for FP64 too.