This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions
AbandonedPublic

Authored by zbuljan on Aug 24 2015, 5:04 AM.

Details

Summary

The patch implements microMIPSr6 16-bit ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions.
Tests for instructions are added in previous patch.

Diff Detail

Event Timeline

zbuljan updated this revision to Diff 32948.Aug 24 2015, 5:04 AM
zbuljan retitled this revision from to [mips][microMIPS] Implement ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
vsk added a subscriber: vsk.Aug 24 2015, 5:20 PM

I have one nit (inline). Could you add a test as well?

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
841

This comment is identical to the one of line 846. Could you reference the fact that you are calling auto-generated decoder functions once (maybe before the if)?

zbuljan updated this revision to Diff 33053.Aug 25 2015, 1:10 AM

Comments updated with more information.
Tests for instructions are added in previous patch (http://reviews.llvm.org/D10955).

zoran.jovanovic accepted this revision.Aug 25 2015, 1:40 AM
zoran.jovanovic edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 25 2015, 1:40 AM
dsanders edited edge metadata.Aug 25 2015, 3:48 AM

Tests for instructions are added in previous patch (http://reviews.llvm.org/D10955).

There ought to be at least one test that would fail without this patch. Are these microMIPSR6 instructions identical to the microMIPS(R3) equivalents?

I looked through the docs in detail. These microMIPSR6 instructions are identical to their microMIPS(R3) equivalents.

We only added tests in previous patch because opcodes of microMIPSR6 instructions ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP are identical to their microMIPS(R3) equivalents and all tests have passed.
Later, we made another patch that changes decoder namespace.
If we commit that patch, disassembler tests for ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions would fail.
So we realized that we have to make new patch which implements ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions.
If we commit this patch which implements ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions and after that patch that changes decoder namespace all tests pass.

Is it OK to commit this patch?

We only added tests in previous patch because opcodes of microMIPSR6 instructions ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP are identical to their microMIPS(R3) equivalents and all tests have passed.
Later, we made another patch that changes decoder namespace.
If we commit that patch, disassembler tests for ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions would fail.

I'm not sure I've seen that patch, could you give me a link to it? I'm not keen to duplicate instruction definitions if we don't have to.

Generally speaking, it's preferable to avoid needing fixup commits (they complicate reverts) so it sounds like this patch (D12279) should be folded into the one you mention and both patches should be committed in a single commit.

The code which adds new decoder table is placed in this patch, D12279, together with implementation of ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions so I it can be commited alone.

The patch D11181 also contained code for decoder table in its first version but is updated (code for decoder table is removed) after revision and you accepted it. Also dependancy is added so it now depends on patch D12279 and will be commited after it.

In my opinion it is safe to firstly commit patch D12279 and then D11181.

We seem to be talking about different things.

To summarize:
The problem with D12279 is that there are no tests in the patch. You've told me that the tests were committed in an earlier patch (rL245554). However, those tests pass without needing the implementations in this patch. This begs the question 'Why do we need these implementations?' and your comment that the microMIPS(R3) and microMIPSR6 encodings are identical hint that the answer is "we don't, it's unnecessary duplication". When questioned about this, your answer was that another patch changed the DecoderNamespace and referenced D11181 which doesn't change any DecoderNamespace's.

My position is that, I don't want duplicate instructions without a very good reason. If the microMIPS(R3) instructions are correct, then we should use them to handle microMIPSR6 as well. Don't forget that the architecture predicates are cumulative, so all microMIPS(R3) instructions are enabled for microMIPSR6 too unless you explicitly prevent this like the NotMips32r6 predicate and the ISA_MIPS32R2_NOT_32R6_64R6 adjective do for the standard encodings.

We had to implement instructions because we introduced new decoder table for microMIPS32R6 16 bit instructions.
Introduction of new decoder table resulted in fail of disassembler tests for ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP instructions.
Our solution was to add microMIPS32R6 implementation of this instructions.

Patch D11181 added new decoder table in it's first version, but later, after your first revision, we moved that code to patch D12279 (together with implementation).
Please can you check all diffs of patch D11181 (there is two diffs: Diff 1 29653 and Diff 2 32951).

New decoder table is also needed for instructions which are implemented in patch D11181 so we added dependancy to it.

I'm pretty sure I see the problem. Not sure why it didn't see it before.

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
839–850

This is incorrectly picking between two tables. It should be trying the DecoderTableMicroMipsR616, then if that fails it should try DecoderTableMicroMips16, then if that fails it should try DecoderTableMicroMipsR632, then DecoderTableMicroMips32, and so on.

See line 904 and 934 for the corresponding standard encoding implementation.

zbuljan abandoned this revision.Aug 28 2015, 2:50 AM

Implementation is not needed.
microMIPSR6 instructions ADDIUR1SP, ADDIUR2, ADDIUS5 and ADDIUSP are identical to their microMIPS(R3) equivalents.