This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement LWM16, SB16, SH16, SW16, SWSP and SWM16 instructions
ClosedPublic

Authored by zbuljan on Jul 22 2015, 2:08 AM.

Details

Summary

The patch implements microMIPSr6 LWM16, SB16, SH16, SW16, SWSP and SWM16 instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

zbuljan updated this revision to Diff 30330.Jul 22 2015, 2:08 AM
zbuljan retitled this revision from to [mips][microMIPS] Implement LWM16, SB16, SH16, SW16, SWSP and SWM16 instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
zbuljan updated this revision to Diff 34151.Sep 7 2015, 5:55 AM

Added support to register list for 64-bit registers.
Subsets of PredicateControl used instead of the Predicates list.

dsanders requested changes to this revision.Sep 16 2015, 6:13 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
983–987 ↗(On Diff #34151)

Can this be written more clearly? It's rather difficult to follow.

Also, why not use both front() and back() rather than mixing begin() and back()?

2579 ↗(On Diff #34151)

' || hasMips64r6()' is redundant since predicates are cumulative.

3978–3979 ↗(On Diff #34151)

Do the registers mentioned here contain data or pointers? The reason I ask is that if it's a pointer then isGP64bit() isn't the right predicate (it should be ArePtrs64bit())

If the answer is 'either', then sticking with isGP64bit() is wrong but it's less wrong than ArePtrs64bit() and there's no right answer without knowing the data type.

Likewise for the other registers below.

4004–4011 ↗(On Diff #34151)

Can we write this more clearly? It's getting rather big and looks like it could be simpler.

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
1873–1874 ↗(On Diff #34151)

Brace belongs on same line as the switch

lib/Target/Mips/MicroMips32r6InstrFormats.td
45 ↗(On Diff #34151)

Should begin with POOL16C

lib/Target/Mips/MicroMips32r6InstrInfo.td
547 ↗(On Diff #34151)

This isn't for your patch but it would be good for 'mem_mm_4sp' to indicate the offset is unsigned.

767 ↗(On Diff #34151)

Also not for your patch but it would be good for addrimm4lsl2 to indicate that it's unsigned too.

lib/Target/Mips/MicroMipsInstrInfo.td
530 ↗(On Diff #34151)

This belongs in the format class. Likewise below

This revision now requires changes to proceed.Sep 16 2015, 6:13 AM
zbuljan updated this revision to Diff 36508.Oct 5 2015, 6:22 AM
zbuljan edited edge metadata.

Used front() instead of begin() in isRegList16() method.
Removed hasMips64r6() from method expandLoadStoreMultiple() because it is not needed.
Registers may contain data so isGP64bit() is left in parseRegisterList() method.
Added POOL16C to format class name.
PredicateControl moved to format class LWM_FM_MM16.

dsanders accepted this revision.Nov 12 2015, 3:40 AM
dsanders edited edge metadata.

LGTM. There was one unanswered question but after re-reading this patch I'm of the opinion that there's no correct answer and using isGP64bit() as the condition is the most correct answer possible.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
4174–4176 ↗(On Diff #36508)

Do the registers mentioned here contain data or pointers? The reason I ask is that if it's a pointer then isGP64bit() isn't the
right predicate (it should be ArePtrs64bit())

If the answer is 'either', then sticking with isGP64bit() is wrong but it's less wrong than ArePtrs64bit() and there's no right
answer without knowing the data type.

Likewise for the other registers below.

I haven't seen an answer to this question but after re-reading I'm pretty sure the answer is 'either'.

This revision is now accepted and ready to land.Nov 12 2015, 3:40 AM
This revision was automatically updated to reflect the committed changes.