[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.

> Added DMUL macro for non CnMips. Please, take a look at the predicates of DMULMacro, and confirm are they OK.


Those predicates look ok to me.

As future work, I think we might want to consider splitting InsnPredicates into a list of positive predicates and a list of negative predicates. Then we could use something like:
def ... : ..., ISA_MIPS64, ASE_NOT_CNMIPS.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3593-3596
@@ +3592,6 @@
+  loadImmediate(ImmValue, ATReg, Mips::NoRegister, true, false, Inst.getLoc(), Instructions);
+
+  unsigned Opcode = Mips::NOP;
+  (Inst.getOpcode() == Mips::MULImm) ? Opcode = Mips::MULT : Opcode = Mips::DMULT;
+  emitRR(Opcode, SReg, ATReg, IDLoc, Instructions);
+
----------------
Sorry, this isn't quite what I meant. I should have been clearer.

Rather than a ternary operator with assignments inside it, I was looking for something like:
  emitRR(Inst.getOpcode() == Mips::MULImm ? Mips::MULT : Mips::DMULT, SReg, ATReg, IDLoc, Instructions);

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3613-3616
@@ +3612,6 @@
+    return true;
+
+  unsigned Opcode = Mips::NOP;
+
+  (Inst.getOpcode() == Mips::MULOMacro) ? Opcode = Mips::MULT : Opcode = Mips::DMULT;
+  emitRR(Opcode, SReg, TReg, IDLoc, Instructions);
----------------
As above, this isn't quite what I meant.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3618-3621
@@ +3617,6 @@
+  emitRR(Opcode, SReg, TReg, IDLoc, Instructions);
+
+  emitR(Mips::MFLO, DReg, IDLoc, Instructions);
+
+  (Inst.getOpcode() == Mips::MULOMacro) ? Opcode = Mips::SRA : Opcode = Mips::DSRA32;
+  emitRRI(Opcode, DReg, DReg, 0x1F, IDLoc, Instructions);
----------------
As above, this isn't quite what I meant.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3628
@@ +3627,3 @@
+  } else {
+    emitRRI(Mips::BEQ, DReg, ATReg, 8, IDLoc, Instructions);
+    if (AssemblerOptions.back()->isReorder())
----------------
I've just noticed that this isn't going to work for micromips because the offset is likely to be different for that case. If it's a constant offset then we can work around it for now (and leave a FIXME) but if it's variable then we'll need to find some way to do this.

For reference, my overall plan for solving this kind of problem is to finish rewriting the emit*() functions such that they directly write to the target streamer. This allows us to drop labels wherever we need them using the target streamer methods.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3643-3646
@@ +3642,6 @@
+  unsigned SReg = Inst.getOperand(1).getReg();
+  unsigned TReg = Inst.getOperand(2).getReg();
+
+  ATReg = getATReg(Inst.getLoc());
+  if (!ATReg)
+    return true;
----------------
As above, this isn't quite what I meant.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3659
@@ +3658,3 @@
+  } else {
+    emitRRI(Mips::BEQ, ATReg, Mips::ZERO, 8, IDLoc, Instructions);
+    if (AssemblerOptions.back()->isReorder())
----------------
Similar to expandMulO(), this constant is different on micromips and might not be a constant.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3669
@@ +3668,3 @@
+bool MipsAsmParser::expandDMULMacro(MCInst &Inst, SMLoc IDLoc,
+                               SmallVectorImpl<MCInst> &Instructions) {
+
----------------
Indentation

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1816-1833
@@ -1813,1 +1815,20 @@
 
+def MULImm : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rd, GPR32Opnd:$rs, simm16:$imm),
+                               "mul\t$rd, $rs, $imm">;
+def MULOMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rd, GPR32Opnd:$rs, GPR32Opnd:$rt),
+                                  "mulo\t$rd, $rs, $rt">, ISA_MIPS1_NOT_32R6_64R6;
+def MULOUMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rd, GPR32Opnd:$rs, GPR32Opnd:$rt),
+                                   "mulou\t$rd, $rs, $rt">, ISA_MIPS1_NOT_32R6_64R6;
+
+def DMULImm : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, simm16:$imm),
+                                "dmul\t$rs, $rt, $imm">, ISA_MIPS1_NOT_32R6_64R6;
+def DMULOMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, GPR32Opnd:$rd),
+                                   "dmulo\t$rs, $rt, $rd">, ISA_MIPS1_NOT_32R6_64R6;
+def DMULOUMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, GPR32Opnd:$rd),
+                                    "dmulou\t$rs, $rt, $rd">, ISA_MIPS1_NOT_32R6_64R6;
+
+def DMULMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt, GPR32Opnd:$rd),
+                                  "dmul\t$rs, $rt, $rd"> {
+  let InsnPredicates = [HasMips64, NotMips64r6, NotCnMips];
+}
+
----------------
MULImm and DMULImm should end in 'Macro' too


http://reviews.llvm.org/D16807