[PATCH] D11139: [mips] Added support for various EVA ASE instructions.

[Daniel Sanders]

dsanders added a comment.

There's few comments here but it's all fairly minor things.


================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:245-247
@@ +244,5 @@
+static DecodeStatus DecodeMemEVA(MCInst &Inst,
+                              unsigned Insn,
+                              uint64_t Address,
+                              const void *Decoder);
+
----------------
Nit: Indendation

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1099-1101
@@ +1098,5 @@
+static DecodeStatus DecodeMemEVA(MCInst &Inst,
+                              unsigned Insn,
+                              uint64_t Address,
+                              const void *Decoder) {
+  int Offset = SignExtend32<9>(Insn >> 7);
----------------
Nit: Indentation

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1109-1110
@@ -1084,3 +1108,4 @@
+
+   if (Inst.getOpcode() == Mips::SCE)
     Inst.addOperand(MCOperand::createReg(Reg));
 
----------------
Nit: Indentation

================
Comment at: lib/Target/Mips/MipsEVAInstrFormats.td:16-19
@@ +15,6 @@
+
+//===----------------------------------------------------------------------===//
+// Mips instruction group membership adjectives.
+//===----------------------------------------------------------------------===//
+class INSN_EVA { list<Predicate> InsnPredicates = [HasEVA]; }
+
----------------
This belongs with the other INSN_* classes in MipsInstrInfo.td

================
Comment at: lib/Target/Mips/MipsEVAInstrFormats.td:23
@@ +22,3 @@
+
+class LWE_FM<bits<6> op> : StdArch {
+  bits<5> rt;
----------------
Spelling nit: I'm trying to keep the names of new format classes related to their names in the documentation (<major-opcode-name>_<arbitrary>_FM).

Could you call this: SPECIAL3_EVA_LOAD_STORE_FM or similar?

================
Comment at: lib/Target/Mips/MipsEVAInstrFormats.td:37-52
@@ +36,17 @@
+
+class CACHEEOP_FM<bits<6> op> : StdArch {
+  bits<21> addr;
+  bits<5> hint;
+  bits<5> base = addr{20-16};
+  bits<9> offset = addr{8-0};
+
+  bits<32> Inst;
+
+  let Inst{31-26} = 0x1F; //SPECIAL3
+  let Inst{25-21} = base;
+  let Inst{20-16} = hint;
+  let Inst{15-7}  = offset;
+  let Inst{6}     = 0;
+  let Inst{5-0}   = op;
+
+}
----------------
This is the same as LWE_FM above. Could you use that instead?

================
Comment at: lib/Target/Mips/MipsEVAInstrInfo.td:19
@@ +18,3 @@
+// Memory Load/Store EVA
+class LoadEVA<string opstr, DAGOperand RO,
+           InstrItinClass Itin = NoItinerary> :
----------------
Could you follow the conventions from Mips{DSP,MSA,32r6,64r6}InstrInfo.td? These conventions define concrete encodings in *_ENC classes, operation descriptions in *_DESC classes and apply predicates in the final def.

This request is mostly spelling nits and syntax but there is also a change to the inheritance hierarchy. This change is that the format inherits from MipsInst (possibly via another class, see MipsR6Inst) instead of the operation description inheriting from it.

================
Comment at: test/MC/Mips/eva/valid-xfail.s:1
@@ +1,2 @@
+# Instructions that should be valid but currently fail for known reasons (e.g.
+# they aren't implemented yet).
----------------
The xfail status and filename of this are misleading. It's correct that EVA instructions fail to assemble when EVA is not enabled so it shouldn't be an xfail. I think you want a invalid-noeva.s with similar contents that checks the error message produced by each instruction. See invalid-mips1.s for an example.

================
Comment at: test/MC/Mips/eva/valid_R6-xfail.s:1
@@ +1,2 @@
+# Instructions that should be valid but currently fail for known reasons (e.g.
+# they aren't implemented yet).
----------------
These aren't valid for 32r6/64r6. [ls]w[lr] and the EVA equivalents [ls]w[lr]e were removed in 32r6/64r6. I think you want an invalid-*.s to cover them


http://reviews.llvm.org/D11139