Implemented *_DESC, *_ENC classes, decoder/encoder methods.
Instructions: balc, bc, beqc, bnec, beqzc, bnezc, bltzc, bgezc, blezc, bgtzc.
Details
- Reviewers
dsanders vmedic - Commits
- rL208974: [mips][mips64r6] Add compact branch instructions
Diff Detail
Event Timeline
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 |
lib/Target/Mips/Mips32r6InstrInfo.td | ||
---|---|---|
121 | Can you double-check this? I don't think they really do def AT. |
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. |
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. |
In case you are interested, D3761 fixes the same bug but also removes the redundant mask. The SignExtend does the same job.