This is an archive of the discontinued LLVM Phabricator instance.

[mips][micromips] Implement ADDU16, AND16, ANDI16, NOT16, OR16, SLL16 and SRL16 instructions
ClosedPublic

Authored by hvarga on Jul 14 2015, 12:46 AM.

Details

Summary

The patch implements microMIPSr6 ADDU16, AND16, ANDI16, NOT16, OR16, SLL16 and SRL16 instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

hvarga updated this revision to Diff 29648.Jul 14 2015, 12:46 AM
hvarga retitled this revision from to [mips][micromips] Implement ADDU16, AND16, ANDI16, NOT16, OR16, SLL16 and SRL16 instructions.
hvarga updated this object.
hvarga added subscribers: llvm-commits, petarj.
dsanders requested changes to this revision.Aug 18 2015, 5:55 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/Disassembler/MipsDisassembler.cpp
1718–1720 ↗(On Diff #29648)

Nit: Indentation

1721–1724 ↗(On Diff #29648)

Nit: Use ternary operator

lib/Target/Mips/MicroMips32r6InstrFormats.td
294 ↗(On Diff #29648)

Style nit: rd is normally first.

lib/Target/Mips/MicroMips32r6InstrInfo.td
287–291 ↗(On Diff #29648)

There's several issues here:

  • This class belongs in MicroMips32r6InstrFormats.td similar to MipsR6Inst in Mips32r6InstrFormats.td and the formats (*_FM) should inherit from it, not the descriptions (*_DESC)
  • Don't use Predicates directly. Inherit from PredicateControl and use its fields.
  • DecoderNamespace is a property of the format/encoding and not of the description.
  • HasStdEnc is a predicate of the format/encoding and belongs in the EncodingPredicates member of the format, but ...
    • ... HasStdEnc means not microMIPS and not MIPS16. It's not correct to use it for microMIPS.
  • Use ISA_MICROMIPS32R6 on the def since it's a property of the instruction and not the description. Also it's more consistent with the other instructions.
    • I see you've already have ISA_MICROMIPS32R6 on the instructions. The reason the adjective isn't working there is because you aren't using PredicateControl and/or you overrode Predicates.
This revision now requires changes to proceed.Aug 18 2015, 5:55 AM
hvarga updated this revision to Diff 33422.Aug 28 2015, 5:46 AM
hvarga edited edge metadata.

Modified patch according the comments from dsanders.

dsanders accepted this revision.Sep 7 2015, 6:01 AM
dsanders edited edge metadata.

LGTM with some minor changes

lib/Target/Mips/MicroMips32r6InstrInfo.td
321–335 ↗(On Diff #33422)

The *_FM should inherit fro MicroMipsR6Inst16 rather than the *_DESC
Also, Indentation.

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

Please define this with the other adjectives in MipsInstrInfo.td and follow the naming convention used by the others (e.g. ISA_MICROMIPS_NOT_32R6_64R6)

lib/Target/Mips/MicroMipsInstrInfo.td
556–570 ↗(On Diff #33422)

Indentation

This revision is now accepted and ready to land.Sep 7 2015, 6:01 AM
This revision was automatically updated to reflect the committed changes.