[mips] MUL macro variations

[sdardis]

Summary

[mips] MUL macro variations

Adds support for MUL macro variations.

Patch by: Srdjan Obucina

[obucina]

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.

[dsanders]

In 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).

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

3587–3590 ↗

(On Diff #46657)

Use the ternary operator

3607–3610 ↗

(On Diff #46657)

Likewise

3612–3615 ↗

(On Diff #46657)

Likewise

3617–3620 ↗

(On Diff #46657)

What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break

3637–3640 ↗

(On Diff #46657)

Use ternary operator

3643–3646 ↗

(On Diff #46657)

What happens when useTraps() is true? Shouldn't this use a trap instruction instead of a break

lib/Target/Mips/MipsInstrInfo.td

1814–1831 ↗

(On Diff #46657)

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.

1947 ↗

(On Diff #46657)

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

[obucina]

Changes according to the comments

[obucina]

Old file accidentally left in the patch - removed.

DMUL for non CnMips still missing.

[obucina]

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

[obucina]

Ping

[dsanders]

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.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

3593–3596 ↗

(On Diff #47340)

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);

3613–3616 ↗

(On Diff #47340)

As above, this isn't quite what I meant.

3618–3621 ↗

(On Diff #47340)

As above, this isn't quite what I meant.

3628 ↗

(On Diff #47340)

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.

3643–3646 ↗

(On Diff #47340)

As above, this isn't quite what I meant.

3659 ↗

(On Diff #47340)

Similar to expandMulO(), this constant is different on micromips and might not be a constant.

3669 ↗

(On Diff #47340)

Indentation

lib/Target/Mips/MipsInstrInfo.td

1816–1833 ↗

(On Diff #47340)

MULImm and DMULImm should end in 'Macro' too

[obucina]

According to feedback - using ternary operators.

[obucina]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

3691 ↗

(On Diff #51340)

It is unclear to me what do you suggest regarding micromips potential problem with branch offset. Please be more precise.

3722 ↗

(On Diff #51340)

It is unclear to me what do you suggest regarding micromips potential problem with branch offset. Please be more precise.

[dsanders]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

3691 ↗

(On Diff #51340)

The offset needs to be different for microMIPS so you need to test for microMIPS and select between the MIPS and microMIPS value.
The way to fix this depends on whether the offset for the microMIPS case is a constant or is variable.

If the offset for the microMIPS case is a constant then we can use magic numbers for the moment (but please document them with comments). If the offset for the microMIPS case is variable for any reason then we'll have to find some way to account for the variability somehow (which could require the work explained at the bottom of this comment).

Ideally, we'd avoid any magic numbers using symbols and symbol references but it's not possible to do that right now (see below)

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.

The reason we can't use symbols to avoid magic numbers at the moment is because this migration work is not yet complete. The problem is that we collect the expansion in a list of instructions and we can't put symbol definitions in this list. To eliminate the magic numbers we need to finish this work and use the target streamers as a stream. This will allow us to drop temporary symbols wherever we need them.

[seanbruno]

This review seems to have aged out and needs the last comments addressed and a refresh to latest trunk.

[sdardis]

Taking over this patch.

[sdardis]

Updated to trunk, addressed outstanding comment on directly specified branch operands vs. usage of label operands and labels.

[sdardis]

Removed some unnecessary symbol manipulation.

[seanbruno]

hrm ... I guess I can't "approve" this patch or something.

[sdardis]

It requires @dsanders to accept the revision as he's requested changes previously.

[dsanders]

In D16807#667692, @sdardis wrote:

It requires @dsanders to accept the revision as he's requested changes previously.

That must be a new requirement of phabricator. Marked as approved to let it change the overall state.

llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp

#include "MipsGenAsmMatcher.inc"
   bool expandDRotation(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                        const MCSubtargetInfo *STI);
   bool expandDRotationImm(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                           const MCSubtargetInfo *STI);
 
   bool expandAbs(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                  const MCSubtargetInfo *STI);
 
+  bool expandMulImm(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                    const MCSubtargetInfo *STI);
+
+  bool expandMulO(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                  const MCSubtargetInfo *STI);
+
+  bool expandMulOU(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                   const MCSubtargetInfo *STI);
+
+  bool expandDMULMacro(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                       const MCSubtargetInfo *STI);
+
   bool expandLoadStoreDMacro(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                              const MCSubtargetInfo *STI, bool IsLoad);
 
   bool expandSeq(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                  const MCSubtargetInfo *STI);
 
   bool expandSeqI(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                   const MCSubtargetInfo *STI);
MipsAsmParser::tryExpandInstruction(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
   case Mips::DROL:
   case Mips::DROR:
     return expandDRotation(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
   case Mips::DROLImm:
   case Mips::DRORImm:
     return expandDRotationImm(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
   case Mips::ABSMacro:
     return expandAbs(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
+  case Mips::MULImmMacro:
+  case Mips::DMULImmMacro:
+    return expandMulImm(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
+  case Mips::MULOMacro:
+  case Mips::DMULOMacro:
+    return expandMulO(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
+  case Mips::MULOUMacro:
+  case Mips::DMULOUMacro:
+    return expandMulOU(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
+  case Mips::DMULMacro:
+    return expandDMULMacro(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
   case Mips::LDMacro:
   case Mips::SDMacro:
     return expandLoadStoreDMacro(Inst, IDLoc, Out, STI,
                                  Inst.getOpcode() == Mips::LDMacro)
                ? MER_Fail
                : MER_Success;
   case Mips::SEQMacro:
     return expandSeq(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
if (FirstRegOp != SecondRegOp)
     TOut.emitRRR(Mips::ADDu, FirstRegOp, SecondRegOp, Mips::ZERO, IDLoc, STI);
   else
     TOut.emitEmptyDelaySlot(false, IDLoc, STI);
   TOut.emitRRR(Mips::SUB, FirstRegOp, Mips::ZERO, SecondRegOp, IDLoc, STI);
 
   return false;
 }
 
+bool MipsAsmParser::expandMulImm(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                                 const MCSubtargetInfo *STI) {
+  MipsTargetStreamer &TOut = getTargetStreamer();
+  unsigned ATReg = Mips::NoRegister;
+  unsigned DstReg = Inst.getOperand(0).getReg();
+  unsigned SrcReg = Inst.getOperand(1).getReg();
+  int32_t ImmValue = Inst.getOperand(2).getImm();
+
+  ATReg = getATReg(IDLoc);
+  if (!ATReg)
+    return true;
+
+  loadImmediate(ImmValue, ATReg, Mips::NoRegister, true, false, IDLoc, Out, STI);
+
+  TOut.emitRR(Inst.getOpcode() == Mips::MULImmMacro ? Mips::MULT : Mips::DMULT,
+              SrcReg, ATReg, IDLoc, STI);
+
+  TOut.emitR(Mips::MFLO, DstReg, IDLoc, STI);
+
+  return false;
+}
+
+bool MipsAsmParser::expandMulO(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                               const MCSubtargetInfo *STI) {
+  MipsTargetStreamer &TOut = getTargetStreamer();
+  unsigned ATReg = Mips::NoRegister;
+  unsigned DstReg = Inst.getOperand(0).getReg();
+  unsigned SrcReg = Inst.getOperand(1).getReg();
+  unsigned TmpReg = Inst.getOperand(2).getReg();
+
+  ATReg = getATReg(Inst.getLoc());
+  if (!ATReg)
+    return true;
+
+  TOut.emitRR(Inst.getOpcode() == Mips::MULOMacro ? Mips::MULT : Mips::DMULT,
+              SrcReg, TmpReg, IDLoc, STI);
+
+  TOut.emitR(Mips::MFLO, DstReg, IDLoc, STI);
+
+  TOut.emitRRI(Inst.getOpcode() == Mips::MULOMacro ? Mips::SRA : Mips::DSRA32,
+               DstReg, DstReg, 0x1F, IDLoc, STI);
+
+  TOut.emitR(Mips::MFHI, ATReg, IDLoc, STI);
+
+  if (useTraps()) {
+    TOut.emitRRI(Mips::TNE, DstReg, ATReg, 6, IDLoc, STI);
+  } else {
+    MCContext & Context = TOut.getStreamer().getContext();
+    MCSymbol * BrTarget = Context.createTempSymbol();
+    MCOperand LabelOp =
+        MCOperand::createExpr(MCSymbolRefExpr::create(BrTarget, Context));
+
+    TOut.emitRRX(Mips::BEQ, DstReg, ATReg, LabelOp, IDLoc, STI);
+    if (AssemblerOptions.back()->isReorder())
+      TOut.emitNop(IDLoc, STI);
+    TOut.emitII(Mips::BREAK, 6, 0, IDLoc, STI);
+
+    TOut.getStreamer().EmitLabel(BrTarget);
+  }
+  TOut.emitR(Mips::MFLO, DstReg, IDLoc, STI);
+
+  return false;
+}
+
+bool MipsAsmParser::expandMulOU(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                                const MCSubtargetInfo *STI) {
+  MipsTargetStreamer &TOut = getTargetStreamer();
+  unsigned ATReg = Mips::NoRegister;
+  unsigned DstReg = Inst.getOperand(0).getReg();
+  unsigned SrcReg = Inst.getOperand(1).getReg();
+  unsigned TmpReg = Inst.getOperand(2).getReg();
+
+  ATReg = getATReg(IDLoc);
+  if (!ATReg)
+    return true;
+
+  TOut.emitRR(Inst.getOpcode() == Mips::MULOUMacro ? Mips::MULTu : Mips::DMULTu,
+              SrcReg, TmpReg, IDLoc, STI);
+
+  TOut.emitR(Mips::MFHI, ATReg, IDLoc, STI);
+  TOut.emitR(Mips::MFLO, DstReg, IDLoc, STI);
+  if (useTraps()) {
+    TOut.emitRRI(Mips::TNE, ATReg, Mips::ZERO, 6, IDLoc, STI);
+  } else {
+    MCContext & Context = TOut.getStreamer().getContext();
+    MCSymbol * BrTarget = Context.createTempSymbol();
+    MCOperand LabelOp =
+        MCOperand::createExpr(MCSymbolRefExpr::create(BrTarget, Context));
+
+    TOut.emitRRX(Mips::BEQ, ATReg, Mips::ZERO, LabelOp, IDLoc, STI);
+    if (AssemblerOptions.back()->isReorder())
+      TOut.emitNop(IDLoc, STI);
+    TOut.emitII(Mips::BREAK, 6, 0, IDLoc, STI);
+
+    TOut.getStreamer().EmitLabel(BrTarget);
+  }
+
+  return false;
+}
+
+bool MipsAsmParser::expandDMULMacro(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                                    const MCSubtargetInfo *STI) {
+  MipsTargetStreamer &TOut = getTargetStreamer();
+  unsigned DstReg = Inst.getOperand(0).getReg();
+  unsigned SrcReg = Inst.getOperand(1).getReg();
+  unsigned TmpReg = Inst.getOperand(2).getReg();
+
+  TOut.emitRR(Mips::DMULTu, SrcReg, TmpReg, IDLoc, STI);
+  TOut.emitR(Mips::MFLO, DstReg, IDLoc, STI);
+
+  return false;
+}
+
 static unsigned nextReg(unsigned Reg) {
   switch (Reg) {
   case Mips::ZERO: return Mips::AT;
   case Mips::AT:   return Mips::V0;
   case Mips::V0:   return Mips::V1;
   case Mips::V1:   return Mips::A0;
   case Mips::A0:   return Mips::A1;
   case Mips::A1:   return Mips::A2;

llvm/trunk/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

MipsTargetELFStreamer::MipsTargetELFStreamer(MCStreamer &S,
   if (Features[Mips::FeatureNaN2008])
     EFlags |= ELF::EF_MIPS_NAN2008;
 
   MCA.setELFHeaderEFlags(EFlags);
 }
 
 void MipsTargetELFStreamer::emitLabel(MCSymbol *S) {
   auto *Symbol = cast<MCSymbolELF>(S);
-  if (!isMicroMipsEnabled())
-    return;
   getStreamer().getAssembler().registerSymbol(*Symbol);
   uint8_t Type = Symbol->getType();
   if (Type != ELF::STT_FUNC)
     return;
 
+  if (isMicroMipsEnabled())
     Symbol->setOther(ELF::STO_MIPS_MICROMIPS);
 }
 
 void MipsTargetELFStreamer::finish() {
   MCAssembler &MCA = getStreamer().getAssembler();
   const MCObjectFileInfo &OFI = *MCA.getContext().getObjectFileInfo();
 
   // .bss, .text and .data are always at least 16-byte aligned.
   MCSection &TextSection = *OFI.getTextSection();

llvm/trunk/lib/Target/Mips/Mips64InstrInfo.td

class LoadImmediate64<string instr_asm, Operand Od, RegisterOperand RO> :
   MipsAsmPseudoInst<(outs RO:$rt), (ins Od:$imm64),
                      !strconcat(instr_asm, "\t$rt, $imm64")> ;
 def LoadImm64 : LoadImmediate64<"dli", imm64, GPR64Opnd>;
 
 def LoadAddrReg64 : MipsAsmPseudoInst<(outs GPR64Opnd:$rt), (ins mem:$addr),
                                        "dla\t$rt, $addr">;
 def LoadAddrImm64 : MipsAsmPseudoInst<(outs GPR64Opnd:$rt), (ins imm64:$imm64),
                                        "dla\t$rt, $imm64">;
+
+def DMULImmMacro : MipsAsmPseudoInst<(outs), (ins GPR64Opnd:$rs, GPR64Opnd:$rt,
+                                                  simm32_relaxed:$imm),
+                                     "dmul\t$rs, $rt, $imm">,
+                   ISA_MIPS3_NOT_32R6_64R6;
+def DMULOMacro : MipsAsmPseudoInst<(outs), (ins GPR64Opnd:$rs, GPR64Opnd:$rt,
+                                                GPR64Opnd:$rd),
+                                   "dmulo\t$rs, $rt, $rd">,
+                 ISA_MIPS3_NOT_32R6_64R6;
+def DMULOUMacro : MipsAsmPseudoInst<(outs), (ins GPR64Opnd:$rs, GPR64Opnd:$rt,
+                                                 GPR64Opnd:$rd),
+                                    "dmulou\t$rs, $rt, $rd">,
+                  ISA_MIPS3_NOT_32R6_64R6;
+
+def DMULMacro : MipsAsmPseudoInst<(outs), (ins GPR64Opnd:$rs, GPR64Opnd:$rt,
+                                               GPR64Opnd:$rd),
+                                  "dmul\t$rs, $rt, $rd"> {
+  let InsnPredicates = [HasMips3, NotMips64r6, NotCnMips];
+}

llvm/trunk/lib/Target/Mips/MipsInstrInfo.td

 
 def SEQIMacro : MipsAsmPseudoInst<(outs GPR32Opnd:$rd),
                                   (ins GPR32Opnd:$rs, simm32_relaxed:$imm),
                                   "seq $rd, $rs, $imm">, NOT_ASE_CNMIPS;
 
 def : MipsInstAlias<"seq $rd, $imm",
                     (SEQIMacro GPR32Opnd:$rd, GPR32Opnd:$rd, simm32:$imm), 0>,
                     NOT_ASE_CNMIPS;
+
+def MULImmMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rd, GPR32Opnd:$rs,
+                                                 simm32_relaxed:$imm),
+                                    "mul\t$rd, $rs, $imm">,
+                  ISA_MIPS1_NOT_32R6_64R6;
+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;
+
 //===----------------------------------------------------------------------===//
 // Instruction aliases
 //===----------------------------------------------------------------------===//
 def : MipsInstAlias<"move $dst, $src",
                     (OR GPR32Opnd:$dst, GPR32Opnd:$src, ZERO), 1>,
       GPR_32 {
   let AdditionalPredicates = [NotInMicroMips];
 }
let AdditionalPredicates = [NotInMicroMips] in {
   def : MipsInstAlias<"seh $rd", (SEH GPR32Opnd:$rd, GPR32Opnd:$rd), 0>,
                      ISA_MIPS32R2;
   def : MipsInstAlias<"seb $rd", (SEB GPR32Opnd:$rd, GPR32Opnd:$rd), 0>,
                      ISA_MIPS32R2;
 }
 def : MipsInstAlias<"sdbbp", (SDBBP 0)>, ISA_MIPS32_NOT_32R6_64R6;
 def : MipsInstAlias<"sync",
                     (SYNC 0), 1>, ISA_MIPS2;
+
+def : MipsInstAlias<"mulo $rs, $rt",
+                    (MULOMacro GPR32Opnd:$rs, GPR32Opnd:$rs, GPR32Opnd:$rt), 0>,
+                    ISA_MIPS1_NOT_32R6_64R6;
+def : MipsInstAlias<"mulou $rs, $rt",
+                    (MULOUMacro GPR32Opnd:$rs, GPR32Opnd:$rs, GPR32Opnd:$rt), 0>,
+                    ISA_MIPS1_NOT_32R6_64R6;
+
 //===----------------------------------------------------------------------===//
 // Assembler Pseudo Instructions
 //===----------------------------------------------------------------------===//
 
 // We use uimm32_coerced to accept a 33 bit signed number that is rendered into
 // a 32 bit number.
 class LoadImmediate32<string instr_asm, Operand Od, RegisterOperand RO> :
   MipsAsmPseudoInst<(outs RO:$rt), (ins Od:$imm32),