This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement JALRC16, JRCADDIUSP and JRC16 instructions
ClosedPublic

Authored by zbuljan on Jul 15 2015, 6:29 AM.

Details

Summary

The patch implements microMIPSr6 JALRC16, JRCADDIUSP and JRC16 instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

zbuljan updated this revision to Diff 29780.Jul 15 2015, 6:29 AM
zbuljan retitled this revision from to [mips][microMIPS] Implement JALRC16, JRCADDIUSP and JRC16 instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
zbuljan updated this revision to Diff 33677.Sep 1 2015, 5:10 AM

Modified patch according to reviews of accepted patches that also implements microMIPSr6 16-bit instructions.

dsanders requested changes to this revision.Sep 2 2015, 3:33 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
974 ↗(On Diff #33677)

This doesn't sound right. uimm5 is indeed 0-32 but nothing accounts for the Lsl2 part of the name.

Shouldn't this be testing for multiples of 4 between 0 and 128?

lib/Target/Mips/MicroMipsInstrFormats.td
40–45 ↗(On Diff #33677)

All instructions should be inheriting from PredicateControl. There's no reason not to do so and not doing so leaves us with two incompatible ways of managing predicates. If you look at InstSE/MipsR6Inst/etc. (which inherit from PredicateControl) you'll see that all the instruction formats inherit from one of them. It seems microMIPS(R3) (and MIPS16?) still needs to have PredicateControl added to the formats but we should ensure that microMIPSR6 is consistent with the better practice used by everything else.

Also, please do this as an adjective (like ISA_MIPS64_NOT_64R6) and put it with the others in MipsInstrInfo.td

This revision now requires changes to proceed.Sep 2 2015, 3:33 AM
mpf added a subscriber: mpf.Sep 2 2015, 3:57 AM
mpf added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
974 ↗(On Diff #33677)

Don't you mean 0-31 inclusive? and therefore 0->124 inclusive in multiples of 4?

dsanders added inline comments.Sep 2 2015, 4:03 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
974 ↗(On Diff #33677)

Yes, sorry for the ambiguity.

zbuljan updated this revision to Diff 35856.Sep 28 2015, 5:26 AM
zbuljan edited edge metadata.

Fixed test for operand in isUImm5Lsl2() method.
Added ISA_MICROMIPS32_NOT_MIPS32R6.

dsanders accepted this revision.Sep 30 2015, 9:39 AM
dsanders edited edge metadata.

LGTM with a couple nits.

There's also a comment for a later patch.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
983 ↗(On Diff #35856)

Variables begin with a capital.

984 ↗(On Diff #35856)

Thanks for fixing this.

I haven't tried this function myself but MathExtras.h provides an isShiftedUInt that should do the same thing as this expression. If that does the job then we should use it, otherwise this is fine.

lib/Target/Mips/MicroMipsInstrInfo.td
412 ↗(On Diff #35856)

Thanks. We should move PredicateControl further up the hierarchy in a later patch.

This revision is now accepted and ready to land.Sep 30 2015, 9:39 AM
This revision was automatically updated to reflect the committed changes.