This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement BREAK, EHB and EI instructions
ClosedPublic

Authored by zbuljan on May 28 2015, 8:25 AM.

Details

Summary

The patch implements microMIPSr6 BREAK, EHB and EI instructions.

Diff Detail

Event Timeline

zbuljan updated this revision to Diff 26688.May 28 2015, 8:25 AM
zbuljan retitled this revision from to [mips][microMIPS] Implement BREAK, EHB and EI instructions.
zbuljan updated this object.
zbuljan edited the test plan for this revision. (Show Details)
zbuljan added subscribers: petarj, Unknown Object (MLST).
dsanders added inline comments.Jun 1 2015, 7:07 AM
lib/Target/Mips/MicroMips32r6InstrFormats.td
286

Nit: It's called 'rs' in the docs.

lib/Target/Mips/MicroMips32r6InstrInfo.td
343

Shouldn't EHB's R6MMR6Rel be StdMMR6Rel?

lib/Target/Mips/MipsInstrInfo.td
410–413

Adding a second uimm10 operand is going to get confusing. Please use the existing uimm10 and change that to restrict the accepted immediates like you have here.

I'd recommend changing uimm10 in a separate commit.

test/MC/Disassembler/Mips/micromips.txt
357–362

I know this is from earlier patches, but please change the rest of this file (in a separate commit) to the

0x00 0x0a 0x57 0x7c # CHECK: ei $10

style. Could you also do this for the other micromips disassembler tests?

zbuljan added inline comments.Jun 12 2015, 2:38 AM
lib/Target/Mips/MicroMips32r6InstrFormats.td
286

I think there is some problem with docs. Tests pass with rt but drops with rs. There is also rt in format class EI_FM_MM in MicroMipsInstrFormats.td. Can you check, please.

zbuljan updated this revision to Diff 27922.Jun 18 2015, 1:55 AM
zbuljan edited reviewers, added: hvarga; removed: jkolek.

R6MMR6Rel is changed to StdMMR6Rel in definition of EHB instruction.
Existing uimm10 operand is used instead of new one.
Operand uimm10 is changed to restrict the accepted immediates in separate commit.
Text style in micromips disassembler test files (.txt) is changed to "one-line" style in separate commit.

dsanders accepted this revision.Jun 18 2015, 2:16 AM
dsanders edited edge metadata.

LGTM with the comment added.

lib/Target/Mips/MicroMips32r6InstrFormats.td
286

The failure when you change this isn't a documentation problem. Most likely, you changed this 'rt' to 'rs' but didn't update the 'rt' in EI_MMR6_DESC to match it. Operands are matched to encoding fields by name.

That said, the documentation uses rt for MIPS and rs for microMIPS. Both documents seem correct, the difference in name stems from differences in how the encodings have been assigned to the encoding formats.

I see that we're re-using the MIPS operation description and it would be a shame to duplicate it just to rename the operand. I therefore suggest adding a comment here. Maybe:

bits<5> rt; // Actually rs but we're sharing code with the standard encodings which call it rt.
lib/Target/Mips/MicroMips32r6InstrInfo.td
343

Done

lib/Target/Mips/MipsInstrInfo.td
410–413

Done

This revision is now accepted and ready to land.Jun 18 2015, 2:16 AM
zbuljan closed this revision.Jun 24 2015, 6:34 AM

Closed by commit rL240531: [mips][microMIPS] Implement BREAK, EHB and EI instructions