This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement MTC*, MTHC* and DMTC* instructions
ClosedPublic

Authored by hvarga on Feb 17 2016, 2:22 AM.

Details

Summary

This patch implements microMIPSr6 MTC0, MTC1, MTC2, MTHC0, MTHC1, MTHC2, DMTC0, DMTC1, DMTC2 instructions.

Diff Detail

Event Timeline

hvarga updated this revision to Diff 48163.Feb 17 2016, 2:22 AM
hvarga retitled this revision from to [mips][microMIPS] Implement MTC*, MTHC* and DMTC* instructions.
hvarga updated this object.
hvarga added subscribers: llvm-commits, petarj.
hvarga updated this revision to Diff 48181.Feb 17 2016, 5:47 AM
hvarga edited edge metadata.

Fixed typo in instruction encoding in lib/Target/Mips/MicroMips64r6InstrInfo.td for dmtc1 and dmtc2.

sdardis added a subscriber: sdardis.

I believe the changes to test/MC/Mips/{mips32r5mips32r6,mips64r5,mips64r6}/invalid.s while welcome are unnecessary given that this patch deals with microMIPSR6. Those should be split out as a separate patch.

I've noticed a possible error in the documentation that shows dmtc2 as syntactically taking a selector but having no field for encoding it in the binary instruction. In contrast mtc2 and mthc2 do not syntactically have a selector. I'm awaiting clarification on that issue.

Thanks,
Simon

lib/Target/Mips/MicroMips32r6InstrInfo.td
558

Space after "NoItinerary,"

test/MC/Disassembler/Mips/micromips32r6/valid.txt
266

Missing instruction on the last line. I presume it's mthc2.

Also can you add the cases with no selector, i.e. "mtc0 $5, $9", "mthc0 $5, $9".

test/MC/Disassembler/Mips/micromips64r6/valid.txt
172

No selector case here too.

Also can you add the mt* cases as well here.

test/MC/Mips/micromips64r6/invalid.s
138

Add mtc0, mthc0 cases here.

test/MC/Mips/micromips64r6/valid.s
157

Can you add the mt* cases here too?

sdardis requested changes to this revision.Feb 18 2016, 9:51 AM
sdardis edited edge metadata.
This revision now requires changes to proceed.Feb 18 2016, 9:51 AM
dsanders requested changes to this revision.Feb 19 2016, 4:15 AM
dsanders edited edge metadata.

In addition to Simon's comments ...

lib/Target/Mips/MicroMips32r6InstrInfo.td
582–583

As with the MFHC1 patch, we need a 64-bit FPU version of MTHC1.

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.

hvarga updated this revision to Diff 50257.EditedMar 10 2016, 3:23 AM
hvarga edited edge metadata.

Patch modified according to comments.

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.

I'm surprised by this but it's correct. It's worth mentioning that this only applies to microMIPS though. The assembler has traditionally accepted a sel operand for the other ISA's.

sdardis accepted this revision.Mar 22 2016, 9:54 AM
sdardis edited edge metadata.

LGTM.

This revision was automatically updated to reflect the committed changes.