This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement BOVC, BNVC, EXT, INS and JALRC instructions
ClosedPublic

Authored by hvarga on Aug 6 2015, 4:21 AM.

Details

Summary

The patch implements microMIPS32r6 BOVC, BNVC, EXT, INS and JALRC instructions.

Diff Detail

Event Timeline

hvarga updated this revision to Diff 31437.Aug 6 2015, 4:21 AM
hvarga retitled this revision from to [mips][microMIPS] Implement BOVC, BNVC, EXT, INS, JALRC, PREFE and NAL instructions.
hvarga updated this object.
hvarga added subscribers: petarj, llvm-commits.
dsanders edited edge metadata.Sep 16 2015, 5:09 AM

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
880

Indentation

lib/Target/Mips/MicroMips32r6InstrFormats.td
124

simm12 -> simm9

523–567

Naming convention. These are missing the major opcode prefix:

EXT/INS => POOL32A
PREFE => POOL32C
JALRC => POOL32A
lib/Target/Mips/MicroMips32r6InstrInfo.td
1207–1253

As noted in other patches, you don't need to separate *_DESC_BASE from *_DESC if there's only one subclass.

1208–1209

Indentation

1222

Indentation

1228–1229

Indentation

1242

Indentation

lib/Target/Mips/MicroMipsInstrInfo.td
677

Indentation (should line up with the first char of MMRel)

lib/Target/Mips/MipsInstrInfo.td
2004–2007

Indentation

hvarga retitled this revision from [mips][microMIPS] Implement BOVC, BNVC, EXT, INS, JALRC, PREFE and NAL instructions to [mips][microMIPS] Implement BOVC, BNVC, EXT, INS and JALRC instructions.Nov 17 2015, 10:56 PM
hvarga updated this object.
hvarga edited edge metadata.
hvarga updated this revision to Diff 40473.Nov 17 2015, 11:00 PM

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.

dsanders requested changes to this revision.Dec 4 2015, 7:51 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/Disassembler/MipsDisassembler.cpp
617–619

BOVC/BEQC/BEQZALC aren't in the ADDI opcode group in microMIPS. The documentation says they're in the POP35 group

Also, indentation.

687–689

Similarly, BNVC/BNEC/BNEZALC are in the POP37 group rather than DADDI.

Also, indentation

lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
263–264

Indentation

269

The return statement should be on the next line

lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.h
109–110

Indentation

lib/Target/Mips/MicroMips32r6InstrFormats.td
523

This should inherit from MipsR6Inst and MMR6Arch<instr_asm>

lib/Target/Mips/MicroMips32r6InstrInfo.td
1163

80 cols?

1172

I believe $rs is the only input (like JALR). What value are you expecting to read from $rt/$dst?

1181

DAGOperand -> Operand

1613–1614

This alias isn't correct for all contexts (particularly .set noreorder) and the correctness rules appear to be complex and context dependent.
Could you defer this alias to a later patch that handles all the compact branch aliases?

lib/Target/Mips/MicroMipsInstrInfo.td
673–674

Please move this back where it was. It used to be grouped with the other jumps.

This revision now requires changes to proceed.Dec 4 2015, 7:51 AM
sdardis added a subscriber: sdardis.

Ping?

hvarga updated this revision to Diff 56118.May 4 2016, 3:42 AM
hvarga edited edge metadata.

Updated the patch according to the comments received from dsanders.

sdardis requested changes to this revision.May 10 2016, 9:11 AM
sdardis edited edge metadata.

Can you add invalid tests for these instructions? More comments are inlined.

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
438

Indentation, align "const void *Decoder" with "MCInst &MI" above.

448

Here too.

618–619

Indentation.

688–689

Indentation.

lib/Target/Mips/MicroMips32r6InstrFormats.td
87

Typo, BNVC not BOVC.

lib/Target/Mips/MicroMips32r6InstrInfo.td
204

Typo, Should POP37_BNVC_FM_MMR6 for bnvc.

1212

JARLC also defines RA.

lib/Target/Mips/MicroMipsInstrInfo.td
891

This needs to be marked as not 32R6/64R6.

This revision now requires changes to proceed.May 10 2016, 9:11 AM

Can you add invalid tests for these instructions? More comments are inlined.

hvarga updated this revision to Diff 58939.May 30 2016, 2:19 AM
hvarga edited edge metadata.

Updated according to the comments from sdardis.

hvarga added inline comments.May 30 2016, 2:28 AM
lib/Target/Mips/MicroMipsInstrInfo.td
891

I think that hasMips64r6() is redundant since predicates are cumulative. So I think it is ok to leave this as is. Do you agree?

sdardis requested changes to this revision.May 31 2016, 6:57 AM
sdardis edited edge metadata.

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

You're right. It's fine as is.

test/MC/Mips/micromips32r6/valid.s
347

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.

This revision now requires changes to proceed.May 31 2016, 6:57 AM
hvarga updated this revision to Diff 59836.Jun 6 2016, 11:43 PM
hvarga edited edge metadata.

Updated according to the comments from sdardis.

sdardis accepted this revision.Jun 7 2016, 7:39 AM
sdardis edited edge metadata.

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.

This revision was automatically updated to reflect the committed changes.