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

[zbuljan]

Summary

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

[vsk]

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]

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

[zoran.jovanovic]

LGTM

[dsanders]

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?

[zbuljan]

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

[zbuljan]

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?

[dsanders]

In D12279#233031, @zbuljan wrote:

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.

[zbuljan]

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.

[dsanders]

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.

[zbuljan]

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.

[dsanders]

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

lib/Target/Mips/Disassembler/MipsDisassembler.cpp

839–851

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]

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

lib/Target/Mips/Disassembler/MipsDisassembler.cpp

DecodeStatus MipsDisassembler::getInstruction(MCInst &Instr, uint64_t &Size,
                                               raw_ostream &VStream,
                                               raw_ostream &CStream) const {
   uint32_t Insn;
   DecodeStatus Result;
 
   if (IsMicroMips) {
     Result = readInstruction16(Bytes, Address, Size, Insn, IsBigEndian);
 
+    if (hasMips32r6()) {
+      DEBUG(dbgs() << "Trying MicroMipsR616 table (16-bit instructions):\n");
+      // Calling the auto-generated decoder function for microMIPS32R6
+      // (and microMIPS64R6) 16-bit instructions.
+      Result = decodeInstruction(DecoderTableMicroMipsR616, Instr, Insn,
+                                 Address, this, STI);
+    } else {
       DEBUG(dbgs() << "Trying MicroMips16 table (16-bit instructions):\n");
-    // Calling the auto-generated decoder function.
+      // Calling the auto-generated decoder function for microMIPS 16-bit
+      // instructions.
       Result = decodeInstruction(DecoderTableMicroMips16, Instr, Insn, Address,
                                 this, STI);
+    }
     if (Result != MCDisassembler::Fail) {
       Size = 2;
       return Result;
     }
 
     Result = readInstruction32(Bytes, Address, Size, Insn, IsBigEndian, true);
     if (Result == MCDisassembler::Fail)
       return MCDisassembler::Fail;

lib/Target/Mips/MicroMips32r6InstrFormats.td

 //
 //===----------------------------------------------------------------------===//
 
 class MMR6Arch<string opstr> {
   string Arch = "micromipsr6";
   string BaseOpcode = opstr;
 }
 
+// Class used for microMIPS32r6 and microMIPS64r6 instructions.
+class MicroMipsR6Inst16 : PredicateControl {
+  string DecoderNamespace = "MicroMipsR6";
+  let InsnPredicates = [HasMicroMips32r6];
+}
+
 class POOL32A_BITSWAP_FM_MMR6<bits<6> funct> : MipsR6Inst {
   bits<5> rd;
   bits<5> rt;
 
   bits<32> Inst;
 
   let Inst{31-26} = 0b000000;
   let Inst{25-21} = rt;

lib/Target/Mips/MicroMips32r6InstrInfo.td

 //
 // Instruction Encodings
 //
 //===----------------------------------------------------------------------===//
 class ADD_MMR6_ENC : ARITH_FM_MMR6<"add", 0x110>;
 class ADDIU_MMR6_ENC : ADDI_FM_MMR6<"addiu", 0xc>;
 class ADDU_MMR6_ENC : ARITH_FM_MMR6<"addu", 0x150>;
 class ADDIUPC_MMR6_ENC : PCREL19_FM_MMR6<0b00>;
+class ADDIUR1SP_MMR6_ENC : ADDIUR1SP_FM_MM16, MicroMipsR6Inst16;
+class ADDIUR2_MMR6_ENC : ADDIUR2_FM_MM16, MicroMipsR6Inst16;
+class ADDIUS5_MMR6_ENC : ADDIUS5_FM_MM16, MicroMipsR6Inst16;
+class ADDIUSP_MMR6_ENC : ADDIUSP_FM_MM16, MicroMipsR6Inst16;
 class ALUIPC_MMR6_ENC : PCREL16_FM_MMR6<0b11111>;
 class AND_MMR6_ENC : ARITH_FM_MMR6<"and", 0x250>;
 class ANDI_MMR6_ENC : ADDI_FM_MMR6<"andi", 0x34>;
 class AUIPC_MMR6_ENC  : PCREL16_FM_MMR6<0b11110>;
 class ALIGN_MMR6_ENC : POOL32A_ALIGN_FM_MMR6<0b011111>;
 class AUI_MMR6_ENC : AUI_FM_MMR6;
 class BALC_MMR6_ENC  : BRANCH_OFF26_FM<0b101101>;
 class BC_MMR6_ENC : BRANCH_OFF26_FM<0b100101>;
class SWE_MMR6_DESC_BASE<string opstr, DAGOperand RO, DAGOperand MO,
   InstSE<(outs), (ins RO:$rt, MO:$addr), !strconcat(opstr, "\t$rt, $addr"),
          [(OpNode RO:$rt, Addr:$addr)], Itin, FrmI, opstr> {
   let DecoderMethod = "DecodeMem";
   let mayStore = 1;
 }
 class SW_MMR6_DESC : Store<"sw", GPR32Opnd>;
 class SWE_MMR6_DESC : SWE_MMR6_DESC_BASE<"swe", GPR32Opnd, mem_simm9gpr>;
 
+class ADDIUR1SP_MMR6_DESC : AddImmUR1SP<"addiur1sp", GPRMM16Opnd>,
+                            MMR6Arch<"addiur1sp">;
+class ADDIUR2_MMR6_DESC : AddImmUR2<"addiur2", GPRMM16Opnd>,
+                          MMR6Arch<"addiur2">;
+class ADDIUS5_MMR6_DESC : AddImmUS5<"addius5", GPR32Opnd>, MMR6Arch<"addius5">;
+class ADDIUSP_MMR6_DESC : AddImmUSP<"addiusp">, MMR6Arch<"addiusp">;
+
 //===----------------------------------------------------------------------===//
 //
 // Instruction Definitions
 //
 //===----------------------------------------------------------------------===//
 
 let DecoderNamespace = "MicroMipsR6" in {
 def ADD_MMR6 : StdMMR6Rel, ADD_MMR6_DESC, ADD_MMR6_ENC, ISA_MICROMIPS32R6;
 def ADDIU_MMR6 : StdMMR6Rel, ADDIU_MMR6_DESC, ADDIU_MMR6_ENC, ISA_MICROMIPS32R6;
 def ADDU_MMR6 : StdMMR6Rel, ADDU_MMR6_DESC, ADDU_MMR6_ENC, ISA_MICROMIPS32R6;
 def ADDIUPC_MMR6 : R6MMR6Rel, ADDIUPC_MMR6_ENC, ADDIUPC_MMR6_DESC,
                    ISA_MICROMIPS32R6;
+def ADDIUR1SP_MMR6 : StdMMR6Rel, ADDIUR1SP_MMR6_DESC, ADDIUR1SP_MMR6_ENC,
+                     ISA_MICROMIPS32R6;
+def ADDIUR2_MMR6 : StdMMR6Rel, ADDIUR2_MMR6_DESC, ADDIUR2_MMR6_ENC,
+                   ISA_MICROMIPS32R6;
+def ADDIUS5_MMR6 : StdMMR6Rel, ADDIUS5_MMR6_DESC, ADDIUS5_MMR6_ENC,
+                   ISA_MICROMIPS32R6;
+def ADDIUSP_MMR6 : StdMMR6Rel, ADDIUSP_MMR6_DESC, ADDIUSP_MMR6_ENC,
+                   ISA_MICROMIPS32R6;
 def ALUIPC_MMR6 : R6MMR6Rel, ALUIPC_MMR6_ENC, ALUIPC_MMR6_DESC,
                   ISA_MICROMIPS32R6;
 def AND_MMR6 : StdMMR6Rel, AND_MMR6_DESC, AND_MMR6_ENC, ISA_MICROMIPS32R6;
 def ANDI_MMR6 : StdMMR6Rel, ANDI_MMR6_DESC, ANDI_MMR6_ENC, ISA_MICROMIPS32R6;
 def AUIPC_MMR6 : R6MMR6Rel, AUIPC_MMR6_ENC, AUIPC_MMR6_DESC, ISA_MICROMIPS32R6;
 def ALIGN_MMR6 : R6MMR6Rel, ALIGN_MMR6_ENC, ALIGN_MMR6_DESC, ISA_MICROMIPS32R6;
 def AUI_MMR6 : R6MMR6Rel, AUI_MMR6_ENC, AUI_MMR6_DESC, ISA_MICROMIPS32R6;
 def BALC_MMR6 : R6MMR6Rel, BALC_MMR6_ENC, BALC_MMR6_DESC, ISA_MICROMIPS32R6;