The patch implements microMIPS32r6 BOVC, BNVC, EXT, INS and JALRC instructions.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Your commit message and patch don't agree with each other since there's no mention of BOVC, BNVC, and NAL in the patch. Is some of the patch missing?
lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp | ||
---|---|---|
801 ↗ | (On Diff #31437) | Indentation |
lib/Target/Mips/MicroMips32r6InstrFormats.td | ||
21 ↗ | (On Diff #31437) | simm12 -> simm9 |
233–277 ↗ | (On Diff #31437) | Naming convention. These are missing the major opcode prefix: EXT/INS => POOL32A PREFE => POOL32C JALRC => POOL32A |
lib/Target/Mips/MicroMips32r6InstrInfo.td | ||
284–330 ↗ | (On Diff #31437) | As noted in other patches, you don't need to separate *_DESC_BASE from *_DESC if there's only one subclass. |
285–286 ↗ | (On Diff #31437) | Indentation |
299 ↗ | (On Diff #31437) | Indentation |
305–306 ↗ | (On Diff #31437) | Indentation |
319 ↗ | (On Diff #31437) | Indentation |
lib/Target/Mips/MicroMipsInstrInfo.td | ||
615 ↗ | (On Diff #31437) | Indentation (should line up with the first char of MMRel) |
lib/Target/Mips/MipsInstrInfo.td | ||
1501–1504 ↗ | (On Diff #31437) | Indentation |
Removed the PREFE and NAL from review name. PREFE is already implemented and available in codebase. NAL instruction is not required in microMIPS since revision 6.01.
Instructions BOVC and BNVC are now fully implemented.
lib/Target/Mips/Disassembler/MipsDisassembler.cpp | ||
---|---|---|
608–610 ↗ | (On Diff #40473) | BOVC/BEQC/BEQZALC aren't in the ADDI opcode group in microMIPS. The documentation says they're in the POP35 group Also, indentation. |
678–680 ↗ | (On Diff #40473) | Similarly, BNVC/BNEC/BNEZALC are in the POP37 group rather than DADDI. Also, indentation |
lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp | ||
245–246 ↗ | (On Diff #40473) | Indentation |
251 ↗ | (On Diff #40473) | The return statement should be on the next line |
lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.h | ||
109–110 ↗ | (On Diff #40473) | Indentation |
lib/Target/Mips/MicroMips32r6InstrFormats.td | ||
517 ↗ | (On Diff #40473) | This should inherit from MipsR6Inst and MMR6Arch<instr_asm> |
lib/Target/Mips/MicroMips32r6InstrInfo.td | ||
932 ↗ | (On Diff #40473) | 80 cols? |
941 ↗ | (On Diff #40473) | I believe $rs is the only input (like JALR). What value are you expecting to read from $rt/$dst? |
950 ↗ | (On Diff #40473) | DAGOperand -> Operand |
1240–1241 ↗ | (On Diff #40473) | This alias isn't correct for all contexts (particularly .set noreorder) and the correctness rules appear to be complex and context dependent. |
lib/Target/Mips/MicroMipsInstrInfo.td | ||
669–670 ↗ | (On Diff #40473) | Please move this back where it was. It used to be grouped with the other jumps. |
Can you add invalid tests for these instructions? More comments are inlined.
lib/Target/Mips/Disassembler/MipsDisassembler.cpp | ||
---|---|---|
428 ↗ | (On Diff #56118) | Indentation, align "const void *Decoder" with "MCInst &MI" above. |
438 ↗ | (On Diff #56118) | Here too. |
608–609 ↗ | (On Diff #56118) | Indentation. |
678–679 ↗ | (On Diff #56118) | Indentation. |
lib/Target/Mips/MicroMips32r6InstrFormats.td | ||
87 ↗ | (On Diff #56118) | Typo, BNVC not BOVC. |
lib/Target/Mips/MicroMips32r6InstrInfo.td | ||
193 ↗ | (On Diff #56118) | Typo, Should POP37_BNVC_FM_MMR6 for bnvc. |
1167 ↗ | (On Diff #56118) | JARLC also defines RA. |
lib/Target/Mips/MicroMipsInstrInfo.td | ||
851 ↗ | (On Diff #56118) | This needs to be marked as not 32R6/64R6. |
lib/Target/Mips/MicroMipsInstrInfo.td | ||
---|---|---|
891 ↗ | (On Diff #58939) | I think that hasMips64r6() is redundant since predicates are cumulative. So I think it is ok to leave this as is. Do you agree? |
Needs one more thing: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp:encodeInstruction/LowerCompactBranch needs to be extended so that the microMIPS bovc/bnvc have their constraints of $rs >= $rt upheld.
You don't need to enforce this constraint for assembly parsing as other tools will silently swap the operands around.
lib/Target/Mips/MicroMipsInstrInfo.td | ||
---|---|---|
891 ↗ | (On Diff #58939) | You're right. It's fine as is. |
test/MC/Mips/micromips32r6/valid.s | ||
347 ↗ | (On Diff #58939) | Add tests where the constraint $rs >= $rt is not upheld, i.e. "bovc $4, $2, 24" but the encoding is the same as the $rs >= $rt case. |
LGTM with one simple change. jalrc cannot have $rt == $rs, so add it to the existing cases in MipsAsmParser::checkTargetMatchPredicate and add an invalid test for it.