This is an archive of the discontinued LLVM Phabricator instance.

[mips][micromips] Implement LD, LLD, LWU, SD, DSRL, DSRL32 and DSRLV instructions
ClosedPublic

Authored by hvarga on Jan 27 2016, 12:12 AM.

Details

Summary

The patch implements microMIPS64r6 LD, LLD, LWU, SD, DSRL, DSRL32 and DSRLV instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

hvarga updated this revision to Diff 46109.Jan 27 2016, 12:12 AM
hvarga retitled this revision from to [mips][micromips] Implement LD, LLD, LWU, SD, DSRL, DSRL32 and DSRLV instructions.
hvarga updated this object.
hvarga added subscribers: llvm-commits, petarj.
sdardis requested changes to this revision.Apr 22 2016, 5:42 AM
sdardis added a reviewer: sdardis.
sdardis added a subscriber: sdardis.

I notice that you've implemented LLD here but I am not seeing any outstanding patches for SCD. Is this an oversight or is there another patch to be submitted to implement that?

lib/Target/Mips/MicroMips64r6InstrFormats.td
88 ↗(On Diff #46109)

Can you put a _ between 2R and OFFSET16? Makes it easier to read.

103 ↗(On Diff #46109)

Here too.

119 ↗(On Diff #46109)

Like my previous comments, and my comment on this class in D16917, can you put a '_' between 2R and SA5?

lib/Target/Mips/Mips64InstrInfo.td
186 ↗(On Diff #46109)

Shouldn't this also be covered by NotInMicroMips?

test/MC/Mips/micromips64r6/invalid.s
146–149 ↗(On Diff #46109)

Can you add some out of range negative values too?

test/MC/Mips/mips64r6/invalid.s
51 ↗(On Diff #46109)

Can you add a negative immediate test case here for dsrl?

This revision now requires changes to proceed.Apr 22 2016, 5:42 AM
hvarga updated this revision to Diff 59690.Jun 6 2016, 12:20 AM
hvarga edited edge metadata.

Updated according to the comments from sdardis.

SCD implementation will follow in a separate patch. Priority is to commit remaining patches.

sdardis requested changes to this revision.Jun 7 2016, 6:49 AM
sdardis edited edge metadata.

One last change, can you get these instructions into the instruction mapping tables? That way some existing optimisations will fire properly, e.g. the patterns around Mips64InstrInfo.cpp:561.

lib/Target/Mips/MicroMips64r6InstrInfo.td
278–280 ↗(On Diff #59690)

Similar to the DROTR change I requested, can you change this so that it takes a uimm6 & modify MipsMCCodeEmitter to pick dsrl/dsrl32 depending on the immediate. See also Daniel's comment in D16917.

lib/Target/Mips/Mips64InstrInfo.td
139–152 ↗(On Diff #59690)

These blocks can be joined.

153–168 ↗(On Diff #59690)

And these too.

608 ↗(On Diff #59690)

Drop this newline

lib/Target/Mips/Mips64r6InstrInfo.td
118 ↗(On Diff #59690)

This also needs to be added to the instruction mapping table.

This revision now requires changes to proceed.Jun 7 2016, 6:49 AM
hvarga updated this revision to Diff 61639.Jun 22 2016, 10:14 PM
hvarga edited edge metadata.
hvarga added a subscriber: mamidzic.

Added MM64R6 case to LowerLargeShift and encodeInstruction for DSRL instruction.
Added BaseopCode and Arch to the classes that were missing it in order for instructions to appear in mapping tables.
Added StdMMR6Rel to LD, LLD, LWU, SD, DSRL, DSRL32, DSRLV
Added R6MMRel to LLD.
Changed FM classes so that they inherit from MipsR6Inst and MMR6Arch.
Changed DESC classes for the instructions so that they don't inherit MipsInst.
Joined NotInMicroMips blocks together where possible.
Changed LLD and LLD_R6 mem operand to mem_simm16.
Changed and added some tests.

This update has been contributed by @mamidzic.

sdardis accepted this revision.Jun 23 2016, 6:16 AM
sdardis edited edge metadata.

LGTM, aside some from nits.

lib/Target/Mips/MicroMipsInstrInfo.td
966 ↗(On Diff #61639)

Line up mem_simm12 under "lwu".

lib/Target/Mips/Mips64InstrInfo.td
692–703 ↗(On Diff #61639)

Join these block together.

lib/Target/Mips/MipsInstrInfo.td
942–949 ↗(On Diff #61639)

Move this block to after MipsMemSimm10AsmOperand.

This revision is now accepted and ready to land.Jun 23 2016, 6:16 AM
This revision was automatically updated to reflect the committed changes.