[PATCH] [mips][microMIPS] Implement BREAK, EHB and EI instructions

Daniel Sanders daniel.sanders at imgtec.com
Thu Jun 18 02:16:47 PDT 2015


LGTM with the comment added.


================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:229
@@ +228,3 @@
+  let Inst{25-21} = 0x00;
+  let Inst{20-16} = rt;
+  let Inst{15-6}  = funct;
----------------
zbuljan wrote:
> dsanders wrote:
> > Nit: It's called 'rs' in the docs.
> I think there is some problem with docs. Tests pass with rt but drops with rs. There is also rt in format class EI_FM_MM in MicroMipsInstrFormats.td. Can you check, please.
The failure when you change this isn't a documentation problem. Most likely, you changed this 'rt' to 'rs' but didn't update the 'rt' in EI_MMR6_DESC to match it. Operands are matched to encoding fields by name.

That said, the documentation uses rt for MIPS and rs for microMIPS. Both documents seem correct, the difference in name stems from differences in how the encodings have been assigned to the encoding formats.

I see that we're re-using the MIPS operation description and it would be a shame to duplicate it just to rename the operand. I therefore suggest adding a comment here. Maybe:
  bits<5> rt; // Actually rs but we're sharing code with the standard encodings which call it rt.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:260
@@ -251,1 +259,3 @@
 def DIVU_MMR6 : R6MMR6Rel, DIVU_MMR6_DESC, DIVU_MMR6_ENC, ISA_MICROMIPS32R6;
+def EHB_MMR6 : R6MMR6Rel, EHB_MMR6_DESC, EHB_MMR6_ENC, ISA_MICROMIPS32R6;
+def EI_MMR6 : StdMMR6Rel, EI_MMR6_DESC, EI_MMR6_ENC, ISA_MICROMIPS32R6;
----------------
dsanders wrote:
> Shouldn't EHB's R6MMR6Rel be StdMMR6Rel?
Done

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:400-403
@@ -393,1 +399,6 @@
+
+def uimm_opnd10 : Operand<i32> {
+  let ParserMatchClass = MipsUImm10AsmOperand;
+}
+
 def simm16_64   : Operand<i64> {
----------------
dsanders wrote:
> Adding a second uimm10 operand is going to get confusing. Please use the existing uimm10 and change that to restrict the accepted immediates like you have here.
> 
> I'd recommend changing uimm10 in a separate commit.
Done

http://reviews.llvm.org/D10090

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list