This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement LDC1, SDC1, LDC2, SDC2, LWC1, SWC1, LWC2 and SWC2 instructions and add CodeGen support
ClosedPublic

Authored by zbuljan on Apr 6 2016, 6:24 AM.

Details

Summary

The patch implements microMIPSr6 LDC1, SDC1, LDC2, SDC2, LWC1, SWC1, LWC2 and SWC2 instructions and adds CodeGen support.

Diff Detail

Repository
rL LLVM

Event Timeline

zbuljan updated this revision to Diff 52781.Apr 6 2016, 6:24 AM
zbuljan retitled this revision from to [mips][microMIPS] Implement LWC1, LWC2, SDC1, SDC2, SWC1 and SWC2 instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
dsanders requested changes to this revision.Apr 8 2016, 8:57 AM
dsanders edited edge metadata.

Could you add some codegen test cases for the new instructions?

By the way, was leaving LDC1/LDC2 out of this patch intentional?

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
1599–1614 ↗(On Diff #52781)

This function is the same as DecodeFMem but with the Reg and Base fields swapped. Could you add a comment saying that this is intentional and correct?

lib/Target/Mips/MicroMips32r6InstrInfo.td
660 ↗(On Diff #52781)

addrDefault does not permit offsets but the instruction supports 11-bit. See addrimm12 for an example that supports 12-bit offsets.

674 ↗(On Diff #52781)

addrDefault -> addr to support the 16-bit offset the instruction allows

688 ↗(On Diff #52781)

As per the LWC2 case, we should use an 11-bit version of addrimm12 to support the 11-bit offset the instruction allows

lib/Target/Mips/MicroMipsInstrFPU.td
145–152 ↗(On Diff #52781)

The DecodeNamespace and Predicates/AdditionalPredicates change makes sense but the DecoderMethod change looks wrong. It's applying microMIPSR6 specific decoder methods to microMIPSR2 instructions.

This revision now requires changes to proceed.Apr 8 2016, 8:57 AM
zbuljan updated this revision to Diff 59832.Jun 6 2016, 11:29 PM
zbuljan retitled this revision from [mips][microMIPS] Implement LWC1, LWC2, SDC1, SDC2, SWC1 and SWC2 instructions to [mips][microMIPS] Implement LDC1, SDC1, LDC2, SDC2, LWC1, SWC1, LWC2 and SWC2 instructions and add CodeGen support.
zbuljan updated this object.
zbuljan edited edge metadata.

Moved implementation of LDC1 and LDC2 to this patch.
Added addrimm11 and addrimm16 complex pattern definitions.
Added DAG patterns (LoadRegImmPat and StoreRegImmPat).
Added operand checkings for LDC*, SDC*, LWC* and SWC* instructions.
Updated isMemWithSimmOffset method for relocation support.
Added tests for the standard encodings and invalid tests.
Updated .ll files with CodeGen tests for microMIPSr6.
Added micromips-lwc1-swc1.ll with CodeGen tests for microMIPSr6.

Please, can someone make a review on this patch?

I'll take a look at this.

sdardis accepted this revision.Jul 4 2016, 3:36 AM
sdardis edited edge metadata.

LGTM with some nits (inlined).

You'll need to add -target-abi n64 for any tests that are targeting micromips64r6, as some of the defaults have changed.

lib/Target/Mips/Mips32r6InstrInfo.td
764 ↗(On Diff #59832)

Rather than moving this line up, can you keep the existing ordering and wrap that line in a NotInMicroMips predicate?

test/CodeGen/Mips/mno-ldc1-sdc1.ll
10 ↗(On Diff #59832)

Since this patch was first posted, FileCheck gained a new option -check-prefixes=. See rL273669 on how to use it.

test/MC/Disassembler/Mips/micromips32r3/valid-el.txt
194 ↗(On Diff #59832)

You can eliminate the near duplicates of what you're adding unless the encoding is some how irregular. i.e. the register field is split into two parts. This goes for the other disassembler tests you're adding.

test/MC/Mips/mips32r6/invalid.s
115 ↗(On Diff #59832)

This test file also requires [ls][wd]dc2/ out of range tests.

test/MC/Mips/mips64r6/invalid.s
107 ↗(On Diff #59832)

This test file also requires [ls][wd]dc2/ out of range tests.

This revision was automatically updated to reflect the committed changes.