Page MenuHomePhabricator

[mips] MFC0, MTC0 changes, COP0 register class definition.
ClosedPublic

Authored by sdardis on Jun 19 2015, 7:11 AM.

Details

Summary

Defined a new register class for coprocessor 0's registers.

The instruction definitions for DMTC0, DMFC0, MFC0 and MTC0 have been changed to reflect this.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 28022.Jun 19 2015, 7:11 AM
sdardis retitled this revision from to [mips] MFC0, MTC0 changes, COP0 register class definition..
sdardis updated this object.
sdardis edited the test plan for this revision. (Show Details)
sdardis added a reviewer: dsanders.
sdardis added a subscriber: Unknown Object (MLST).
dsanders edited edge metadata.Jun 22 2015, 3:16 AM

The change looks correct to me but the patch lacks appropriate test cases. Could you add them?

lib/Target/Mips/MCTargetDesc/MipsOptionRecord.cpp
82–83

Tiny nit: Could you add a small comment that the FPU is COP1? This will make the ordering of the if-statements obvious.

sdardis updated this revision to Diff 28354.Jun 24 2015, 7:51 AM
sdardis edited edge metadata.

Tests for mfc0, mtc0, and double word variants in the disassembler.
Reginfo usage test for COP0.

Is there any else required?

Thanks.

sdardis updated this revision to Diff 28357.Jun 24 2015, 8:23 AM

Apologies, last submission had a broken test.

Thanks. The tests need expanding on a bit (mostly copy/paste into each ISA's test file) and the commit message needs improving (particularly the subject line) but after that it will LGTM. I'm aware you don't have commit access yet, so could you update the revision and I'll commit from there.

To elaborate on the tests that are needed: For the disassembler, the tests you've added need to be duplicated in each ISA's test (test/MC/Disassembler/Mips/$isa/valid-$isa.txt and test/MC/Disassembler/Mips/$isa/valid-$isa-el.txt for big/little endian respectively). I believe that mfc0/mtc0 also need to be included in the tests for the 64-bit ISA's since they are still valid instructions. The tests are laid out like this to make it easy to inspect the differences between the ISA's and manually check against documentation. These files should be sorted by opcode.

For the assembler, we also need to test the encoding is correct. The existing tests for this are in test/MC/Mips/*/valid.s (one for each ISA). These files are sorted alphabetically to make it easy to verify against the documentation and inspect the ISA differences using diff. Lastly, we also need to add to test/MC/Mips/$isa1/invalid-$isa2.s which tests that $isa1 rejects things that were valid for $isa2. In this case, we just need to update the existing test/MC/Mips/*/invalid-mips64.s to check that dmfc0/dmtc0 are rejected with the correct error message. If they are rejected but with the wrong error message then there are also invalid-*-xfail.txt tests that can be used.

lib/Target/Mips/MCTargetDesc/MipsOptionRecord.cpp
84

Nit: LLVM uses '//' comments.

sdardis updated this revision to Diff 28461.Jun 26 2015, 6:12 AM
sdardis updated this object.

Added tests, addressed nit.

sdardis updated this revision to Diff 28556.Jun 26 2015, 6:18 AM

Apologies for spam attached wrong revision, this is the correct one.

dsanders accepted this revision.Jun 26 2015, 8:19 AM
dsanders edited edge metadata.

LGTM. I'm aware you don't have commit access yet so I'll commit this shortly.

I realize I said that disassembler tests are supposed to be sorted by opcode but it looks like they're sorted by mnemonic. You've made the correct change despite my mistake. Thanks.

This revision is now accepted and ready to land.Jun 26 2015, 8:19 AM

In the commit, I've fixed a couple issues that came up when running 'ninja check'.

  • Some little endian disassembler tests failed because they had the same opcode as big endian. I've byte-swapped them.
  • Two disassembler tests failed because their expectation was different for the same opcode as the other ISA's.

I also marked DMFC0 as being part of MIPS3 like the other DM[FT]C0's.

dsanders closed this revision.Jun 27 2015, 8:39 AM