[PATCH] D16807: [mips] MUL macro variations

[Daniel Sanders via llvm-commits]

dsanders requested changes to this revision.
dsanders added a comment.
This revision now requires changes to proceed.

In http://reviews.llvm.org/D16807#341755, @obucina wrote:

> In original test example there is one more instruction
>
> dmul $4, $5, $6
>
> However, there is a DMUL instruction in TD files, available only for Cavium Octeon, with predicate [HasCnMips], so I am getting "error: instruction requires a CPU feature not currently enabled" when I try to process this instruction.
>
> I need advice on how to handle this situation. One solution is to define new DMUL pseudo instruction for everything but CnMips. Existing DMUL appears as a processor specific instruction, and not a general one, so I am wondering about naming conventions, should I leave existing DMUL as is, or rename it to reflect its nature.


Yes, adding a MipsAsmPseudoInst for the non-cnMIPS macro is the right way to go about it.

For the naming convention, instructions use the mnemonic as far as possible. A few instructions have conflicting definitions for the same mnemonic so these have a disambiguating suffix like '_R6', '_MM', or '_64'. For assembly macros, we've been moving towards using the mnemonic with a 'Macro' suffix (e.g. DMULMacro).


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3587-3590
@@ +3586,6 @@
+  loadImmediate(ImmValue, ATReg, Mips::NoRegister, true, false, Inst.getLoc(), Instructions);
+  if (Inst.getOpcode() == Mips::MULImm)
+    emitRR(Mips::MULT, SReg, ATReg, IDLoc, Instructions);
+  else
+    emitRR(Mips::DMULT, SReg, ATReg, IDLoc, Instructions);
+  emitR(Mips::MFLO, DReg, IDLoc, Instructions);
----------------
Use the ternary operator

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3607-3610
@@ +3606,6 @@
+
+  if (Inst.getOpcode() == Mips::MULO)
+    emitRR(Mips::MULT, SReg, TReg, IDLoc, Instructions);
+  else
+    emitRR(Mips::DMULT, SReg, TReg, IDLoc, Instructions);
+  emitR(Mips::MFLO, DReg, IDLoc, Instructions);
----------------
Likewise

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3612-3615
@@ +3611,6 @@
+  emitR(Mips::MFLO, DReg, IDLoc, Instructions);
+  if (Inst.getOpcode() == Mips::MULO)
+    emitRRI(Mips::SRA, DReg, DReg, 0x1F, IDLoc, Instructions);
+  else
+    emitRRI(Mips::DSRA32, DReg, DReg, 0x1F, IDLoc, Instructions);
+  emitR(Mips::MFHI, ATReg, IDLoc, Instructions);
----------------
Likewise

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3617-3620
@@ +3616,6 @@
+  emitR(Mips::MFHI, ATReg, IDLoc, Instructions);
+  emitRRI(Mips::BEQ, DReg, ATReg, 8, IDLoc, Instructions);
+  if (AssemblerOptions.back()->isReorder())
+    createNop(false, IDLoc, Instructions);
+  emitII(Mips::BREAK,6, 0, IDLoc, Instructions);
+  emitR(Mips::MFLO, DReg, IDLoc, Instructions);
----------------
What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3637-3640
@@ +3636,6 @@
+
+  if (Inst.getOpcode() == Mips::MULOU)
+    emitRR(Mips::MULTu, SReg, TReg, IDLoc, Instructions);
+  else
+    emitRR(Mips::DMULTu, SReg, TReg, IDLoc, Instructions);
+  emitR(Mips::MFHI, ATReg, IDLoc, Instructions);
----------------
Use ternary operator

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3643-3646
@@ +3642,6 @@
+  emitR(Mips::MFLO, DReg, IDLoc, Instructions);
+  emitRRI(Mips::BEQ, ATReg, Mips::ZERO, 8, IDLoc, Instructions);
+  if (AssemblerOptions.back()->isReorder())
+    createNop(false, IDLoc, Instructions);
+  emitII(Mips::BREAK, 6, 0, IDLoc, Instructions);
+
----------------
What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1814-1831
@@ -1813,2 +1813,20 @@
 
+def MULImm : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rd, GPR32Opnd:$rs, simm16:$imm),
+                               "mul\t$rd, $rs, $imm">;
+let AdditionalPredicates = [NotMips32r6, NotMips64r6] in {
+  def MULO : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rd, GPR32Opnd:$rs, GPR32Opnd:$rt),
+                               "mulo\t$rd, $rs, $rt">;
+  def MULOU : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rd, GPR32Opnd:$rs, GPR32Opnd:$rt),
+                                "mulou\t$rd, $rs, $rt">;
+}
+
+let AdditionalPredicates = [HasMips64] in {
+  def DMULImm : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, simm16:$imm),
+                                "dmul\t$rs, $rt, $imm">;
+  def DMULO : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, GPR32Opnd:$rd),
+                                "dmulo\t$rs, $rt, $rd">;
+  def DMULOU : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, GPR32Opnd:$rd),
+                                 "dmulou\t$rs, $rt, $rd">;
+}
+
 //===----------------------------------------------------------------------===//
----------------
Naming convention as discussed above.

Also, AdditionalPredicates isn't the right predicate list for ISA predicates. Use the ISA_MIPS1_NOT_32R6_64R6 adjective instead.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1947
@@ +1946,3 @@
+
+let AdditionalPredicates = [NotMips32r6, NotMips64r6] in {
+  def : MipsInstAlias<"mulo $rs, $rt",
----------------
AdditionalPredicates isn't the right predicate list for ISA predicates. Use the ISA_MIPS1_NOT_32R6_64R6 adjective instead.


http://reviews.llvm.org/D16807