Page MenuHomePhabricator

MIPS32r6 compact branch instructions
ClosedPublic

Authored by jkolek on May 9 2014, 6:06 AM.

Details

Summary

Implemented *_DESC, *_ENC classes, decoder/encoder methods.
Instructions: balc, bc, beqc, bnec, beqzc, bnezc, bltzc, bgezc, blezc, bgtzc.

Diff Detail

Event Timeline

jkolek updated this revision to Diff 9246.May 9 2014, 6:06 AM
jkolek retitled this revision from to MIPS32r6 compact branch instructions.
jkolek updated this object.
jkolek edited the test plan for this revision. (Show Details)
jkolek added reviewers: dsanders, vmedic.
jkolek added subscribers: zoran.jovanovic, petarj.
dsanders edited edge metadata.May 9 2014, 7:21 AM

As explained below, I'd like you to move the tests into the valid.s. There's also one minor naming convention issue, but it otherwise LGTM

Please include the full context in future since it makes it much easier to review. 'arc diff' will do this for you and makes it easier for you to upload the patch.

lib/Target/Mips/Mips32r6InstrInfo.td
102–112

It's a minor thing but it would be nice to keep the '_DESC' in the ALL base classes. Best I can think of is s/BRANCH_DESC_BASE/BC_DESC_BASE/ and s/BRANCH_BASE/BRANCH_DESC_BASE/'

156

This belongs in the encoding class

160

same

test/MC/Mips/mips32r6-compact-branch-instructions.s
1–3 ↗(On Diff #9246)

Please add your tests to the valid.s for mips32r6. This enables easy generation of the invalid-$isa.s tests by diffing valid.s's

Also, there's no need to explicitly cover both endians, the byte swapping code is very well tested.

1–3 ↗(On Diff #9246)

Nearly forgot to mention, that they should also be in the mips64r6/valid.s as well

jkolek updated this revision to Diff 9267.May 9 2014, 11:39 AM
jkolek edited edge metadata.
dsanders accepted this revision.May 12 2014, 2:34 AM
dsanders edited edge metadata.

LGTM, thanks.

This revision is now accepted and ready to land.May 12 2014, 2:34 AM
dsanders added inline comments.May 13 2014, 5:39 AM
lib/Target/Mips/Mips32r6InstrInfo.td
121

Can you double-check this? I don't think they really do def AT.

jkolek added inline comments.May 14 2014, 3:09 AM
lib/Target/Mips/Mips32r6InstrInfo.td
121

I have put AT in Defs list of *_DESC_BASE classes of the branch instructions because it is in Defs in the base classes of MIPS32r2 branch instructions where AT is used for long branches. I'm not sure if long branch implementation is necessary in MIPS64r6, and if so, whether AT should be used or not. If you think that AT is not needed here I can remove it.

jkolek updated this revision to Diff 9383.May 14 2014, 4:56 AM
jkolek edited edge metadata.
dsanders added inline comments.May 14 2014, 6:38 AM
lib/Target/Mips/Disassembler/MipsDisassembler.cpp
880

In case you are interested, D3761 fixes the same bug but also removes the redundant mask. The SignExtend does the same job.

lib/Target/Mips/Mips32r6InstrInfo.td
121

That makes sense. It's not needed in this patch but if we did remove it, it may need to come back when we add CodeGen support anyway. I think we should leave it in for now. We should probably add it to the other branches when we add CodeGen support as well.

jkolek closed this revision.May 16 2014, 5:02 AM

Committed revision: 208974.

jkolek added a subscriber: Unknown Object (MLST).Nov 18 2014, 5:50 AM