[mips] seq macro support

[sdardis]

Summary

This patch adds the seq macro.

This partially resolves PR/30381.

Thanks to Sean Bruno for reporting the issue!

[emaste]

Looks right to me. I think you can split the seq and not changes into two separate commits?

[seanbruno]

This seems to not cleanly apply and has a couple of rejects:

[e-mail]:~/clang/llvm % find . -name '*.rej'
./test/CodeGen/Mips/fcopysign-f32-f64.ll.rej
./lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej
./lib/Target/Mips/MipsInstrInfo.td.rej
[e-mail]:~/clang/llvm % cat ./lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej
@@ -2213,6 +2219,10 @@
     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::SEQMacro:
+    return expandSeq(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
+  case Mips::SEQIMacro:
+    return expandSeqI(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
   }
 }
 
[e-mail]:~/clang/llvm % cat ./lib/Target/Mips/MipsInstrInfo.td.rej
@@ -201,6 +201,8 @@
                       AssemblerPredicate<"FeatureMips16">;
 def HasCnMips    :    Predicate<"Subtarget->hasCnMips()">,
                       AssemblerPredicate<"FeatureCnMips">;
+def NotCnMips    :    Predicate<"!Subtarget->hasCnMips()">,
+                      AssemblerPredicate<"!FeatureCnMips">;
 def RelocNotPIC :     Predicate<"!TM.isPositionIndependent()">;
 def RelocPIC    :     Predicate<"TM.isPositionIndependent()">;
 def NoNaNsFPMath :    Predicate<"TM.Options.NoNaNsFPMath">;

[sdardis]

What revision are you trying it against? It's applying mostly clean for me against rL281613. Only the changes to MipsAsmParser.cpp had a offset change (-2).

[seanbruno]

~/clang/llvm %        svn info .
Path: .
Working Copy Root Path: /home/sbruno/clang/llvm
URL: http://llvm.org/svn/llvm-project/llvm/trunk
Relative URL: ^/llvm/trunk
Repository Root: http://llvm.org/svn/llvm-project
Repository UUID: 91177308-0d34-0410-b5e6-96231b3b80d8
Revision: 281608
Node Kind: directory
Schedule: normal
Last Changed Author: sdardis
Last Changed Rev: 281607
Last Changed Date: 2016-09-15 13:13:01 +0000 (Thu, 15 Sep 2016)

[sdardis]

Removed 'not' alias, will add that as a separate patch.

[seanbruno]

I'm unsure if this is a patch/phabricator messing up or if this patch needs to be regenerated.

~/clang/llvm % patch -p0 < ../D24607.diff 
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: lib/Target/Mips/AsmParser/MipsAsmParser.cpp
|===================================================================
|--- lib/Target/Mips/AsmParser/MipsAsmParser.cpp
|+++ lib/Target/Mips/AsmParser/MipsAsmParser.cpp
--------------------------
Patching file lib/Target/Mips/AsmParser/MipsAsmParser.cpp using Plan A...
Hunk #1 succeeded at 252 with fuzz 2 (offset 3 lines).
Hunk #2 failed at 2217.
Hunk #3 succeeded at 3909 (offset 83 lines).
1 out of 3 hunks failed--saving rejects to lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: lib/Target/Mips/MipsInstrInfo.td
|===================================================================
|--- lib/Target/Mips/MipsInstrInfo.td
|+++ lib/Target/Mips/MipsInstrInfo.td
--------------------------
Patching file lib/Target/Mips/MipsInstrInfo.td using Plan A...
Hunk #1 failed at 201.
Hunk #2 succeeded at 351 (offset 27 lines).
Hunk #3 succeeded at 2251 (offset 27 lines).
1 out of 3 hunks failed--saving rejects to lib/Target/Mips/MipsInstrInfo.td.rej

I can hand edit to get this patch in, but thought it was worth noting.

[seanbruno]

This patch needs to be regenerated against the top of tree at the moment. I've had to manually modify the code for the following files recently to test this:

./lib/Target/Mips/AsmParser/MipsAsmParser.cpp.rej
./lib/Target/Mips/MipsInstrInfo.td.rej

[sdardis]

That's odd. OSX's patch (2.5.8) reports that hunks #2, #3 apply with offsets 8 and 29 lines respectively.

For MipsAsmPArser, #2 and #3 apply with 5 and 3 respectively. This is against rL283312.

Is your version of patch sufficiently different?

[seanbruno]

Possibly. I'm not going to stress out about it and you're probably going to be committing this soonish?

[sdardis]

Yes, @vkalintiris should be reviewing this soon hopefully.

[vkalintiris]

We generate different code from the GNU assembler which in some cases is wrong, see inline comment.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

3862–3878 ↗

(On Diff #71780)

The constants manipulation here looks problematic, eg:

seq     $4, $5, -66666

With gas:

lui     at,0xfffe
ori     at,at,0xfb96
xor     a0,a1,at
sltiu   a0,a0,1

With llvm-mc:

addiu   $4, $5, 1130
sltiu   $4, $4, 1

Similarly for:

seq     $4, $5, -2147483648

gas:

lui     at,0x8000
xor     a0,a1,at 
sltiu   a0,a0,1

llvm-mc:

addiu   $4, $5, 0
sltiu   $4, $4, 1

Also, on 64-bit we should generate a daddiu instead of an addiu according to the gas testsuite in binutils.

[sdardis]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

3862–3878 ↗

(On Diff #71780)

Right. It appears that seqi reads immediates like li does. I have a working copy with this addressed and I'll add your examples to the test.

[sdardis]

Address review comments.

[vkalintiris]

I didn't consider all the cases but it seems that the macro expansion is probably fine in terms of correctness. However, the expansion is different from that of GAS' and this will generate different object files unnecessarily. As far as I can tell, GAS handles these macro in 3 different ways depending on the value of the immediate:

1. imm == 0 -> generate just an sltiu,
2. op[1] == 0 -> generate warning and set op[0] = 0
3. • imm in [0, 0x10000) -> xori
   • imm in (-0x8000, 0) -> imm = -imm AND {,d}addiu
   • for every other imm -> loadImmediate && xor.
   • finally emit sltiu.

[sdardis]

Addressed review comments.

[vkalintiris]

LGTM but there's a wrong constant that you should change before committing this.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

3977 ↗

(On Diff #76421)

We want the -0x8000 constant literal here.

4316–4320 ↗

(On Diff #76421)

Can you fix the identation here?

[seanbruno]

Currently required to build the FreeBSD MALTA mips kernels. Thanks for working on this!

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

bool expandDRotationImm(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
                           const MCSubtargetInfo *STI);
 
   bool expandAbs(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);
+
   bool reportParseError(Twine ErrorMsg);
   bool reportParseError(SMLoc Loc, Twine ErrorMsg);
 
   bool parseMemOffset(const MCExpr *&Res, bool isParenExpr);
 
   bool isEvaluated(const MCExpr *Expr);
   bool parseSetMips0Directive();
   bool parseSetArchDirective();
MipsAsmParser::tryExpandInstruction(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
   case Mips::ABSMacro:
     return expandAbs(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;
+  case Mips::SEQIMacro:
+    return expandSeqI(Inst, IDLoc, Out, STI) ? MER_Fail : MER_Success;
   }
 }
 
 bool MipsAsmParser::expandJalWithRegs(MCInst &Inst, SMLoc IDLoc,
                                       MCStreamer &Out,
                                       const MCSubtargetInfo *STI) {
   MipsTargetStreamer &TOut = getTargetStreamer();
 
bool MipsAsmParser::expandLoadStoreDMacro(MCInst &Inst, SMLoc IDLoc,
   } else {
     TOut.emitRRX(Opcode, SecondReg, BaseReg, SecondOffset, IDLoc, STI);
     TOut.emitRRX(Opcode, FirstReg, BaseReg, FirstOffset, IDLoc, STI);
   }
 
   return false;
 }
 
+bool MipsAsmParser::expandSeq(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                              const MCSubtargetInfo *STI) {
+
+  warnIfNoMacro(IDLoc);
+  MipsTargetStreamer &TOut = getTargetStreamer();
+
+  if (Inst.getOperand(1).getReg() != Mips::ZERO &&
+      Inst.getOperand(2).getReg() != Mips::ZERO) {
+    TOut.emitRRR(Mips::XOR, Inst.getOperand(0).getReg(),
+                 Inst.getOperand(1).getReg(), Inst.getOperand(2).getReg(),
+                 IDLoc, STI);
+    TOut.emitRRI(Mips::SLTiu, Inst.getOperand(0).getReg(),
+                 Inst.getOperand(0).getReg(), 1, IDLoc, STI);
+    return false;
+  }
+
+  unsigned Reg = 0;
+  if (Inst.getOperand(1).getReg() == Mips::ZERO) {
+    Reg = Inst.getOperand(2).getReg();
+  } else {
+    Reg = Inst.getOperand(1).getReg();
+  }
+  TOut.emitRRI(Mips::SLTiu, Inst.getOperand(0).getReg(), Reg, 1, IDLoc, STI);
+  return false;
+}
+
+bool MipsAsmParser::expandSeqI(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out,
+                               const MCSubtargetInfo *STI) {
+
+  warnIfNoMacro(IDLoc);
+  MipsTargetStreamer &TOut = getTargetStreamer();
+
+  unsigned Opc;
+  int64_t Imm = Inst.getOperand(2).getImm();
+  unsigned Reg = Inst.getOperand(1).getReg();
+
+  if (Imm == 0) {
+    TOut.emitRRI(Mips::SLTiu, Inst.getOperand(0).getReg(),
+                 Inst.getOperand(1).getReg(), 1, IDLoc, STI);
+    return false;
+  } else {
+
+    if (Reg == Mips::ZERO) {
+      Warning(IDLoc, "comparison is always false");
+      TOut.emitRRR(isGP64bit() ? Mips::DADDu : Mips::ADDu,
+                   Inst.getOperand(0).getReg(), Reg, Reg, IDLoc, STI);
+      return false;
+    }
+
+    if (Imm > -0x8000 && Imm < 0) {
+      Imm = -Imm;
+      Opc = isGP64bit() ? Mips::DADDiu : Mips::ADDiu;
+    } else {
+      Opc = Mips::XORi;
+    }
+  }
+  if (!isUInt<16>(Imm)) {
+    unsigned ATReg = getATReg(IDLoc);
+    if (!ATReg)
+      return true;
+
+    if (loadImmediate(Imm, ATReg, Mips::NoRegister, true, isGP64bit(), IDLoc,
+                      Out, STI))
+      return true;
+
+    TOut.emitRRR(Mips::XOR, Inst.getOperand(0).getReg(),
+                 Inst.getOperand(1).getReg(), ATReg, IDLoc, STI);
+    TOut.emitRRI(Mips::SLTiu, Inst.getOperand(0).getReg(),
+                 Inst.getOperand(0).getReg(), 1, IDLoc, STI);
+    return false;
+  }
+
+  TOut.emitRRI(Opc, Inst.getOperand(0).getReg(), Inst.getOperand(1).getReg(),
+               Imm, IDLoc, STI);
+  TOut.emitRRI(Mips::SLTiu, Inst.getOperand(0).getReg(),
+               Inst.getOperand(0).getReg(), 1, IDLoc, STI);
+  return false;
+}
+
 unsigned
 MipsAsmParser::checkEarlyTargetMatchPredicate(MCInst &Inst,
                                               const OperandVector &Operands) {
   switch (Inst.getOpcode()) {
   default:
     return Match_Success;
   case Mips::DATI:
   case Mips::DAHI:

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

 def HasMicroMips32r6 : Predicate<"Subtarget->inMicroMips32r6Mode()">,
                        AssemblerPredicate<"FeatureMicroMips,FeatureMips32r6">;
 def HasMicroMips64r6 : Predicate<"Subtarget->inMicroMips64r6Mode()">,
                        AssemblerPredicate<"FeatureMicroMips,FeatureMips64r6">;
 def InMips16Mode :    Predicate<"Subtarget->inMips16Mode()">,
                       AssemblerPredicate<"FeatureMips16">;
 def HasCnMips    :    Predicate<"Subtarget->hasCnMips()">,
                       AssemblerPredicate<"FeatureCnMips">;
+def NotCnMips    :    Predicate<"!Subtarget->hasCnMips()">,
+                      AssemblerPredicate<"!FeatureCnMips">;
 def RelocNotPIC :     Predicate<"!TM.isPositionIndependent()">;
 def RelocPIC    :     Predicate<"TM.isPositionIndependent()">;
 def NoNaNsFPMath :    Predicate<"TM.Options.NoNaNsFPMath">;
 def HasStdEnc :       Predicate<"Subtarget->hasStandardEncoding()">,
                       AssemblerPredicate<"!FeatureMips16">;
 def NotDSP :          Predicate<"!Subtarget->hasDSP()">;
 def InMicroMips    :  Predicate<"Subtarget->inMicroMipsMode()">,
                       AssemblerPredicate<"FeatureMicroMips">;

 class INSN_MIPS5_32R2_NOT_32R6_64R6 {
   list<Predicate> InsnPredicates = [HasMips5_32r2, NotMips32r6, NotMips64r6];
 }
 
 class ASE_CNMIPS {
   list<Predicate> InsnPredicates = [HasCnMips];
 }
 
+class NOT_ASE_CNMIPS {
+  list<Predicate> InsnPredicates = [NotCnMips];
+}
+
 class ASE_MIPS64_CNMIPS {
   list<Predicate> InsnPredicates = [HasMips64, HasCnMips];
 }
 
 class ASE_MSA {
   list<Predicate> InsnPredicates = [HasMSA];
 }
 

 def : MipsInstAlias<"dror $rd, $rs",
                     (DROR GPR32Opnd:$rd, GPR32Opnd:$rd, GPR32Opnd:$rs), 0>, ISA_MIPS64;
 def : MipsInstAlias<"dror $rd, $imm",
                     (DRORImm GPR32Opnd:$rd, GPR32Opnd:$rd, simm16:$imm), 0>, ISA_MIPS64;
 
 def ABSMacro : MipsAsmPseudoInst<(outs GPR32Opnd:$rd), (ins GPR32Opnd:$rs),
                                  "abs\t$rd, $rs">;
 
+def SEQMacro : MipsAsmPseudoInst<(outs GPR32Opnd:$rd),
+                                 (ins GPR32Opnd:$rs, GPR32Opnd:$rt),
+                                 "seq $rd, $rs, $rt">, NOT_ASE_CNMIPS;
+
+def : MipsInstAlias<"seq $rd, $rs",
+                    (SEQMacro GPR32Opnd:$rd, GPR32Opnd:$rd, GPR32Opnd:$rs), 0>,
+                    NOT_ASE_CNMIPS;
+
+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;
 //===----------------------------------------------------------------------===//
 // Instruction aliases
 //===----------------------------------------------------------------------===//
 def : MipsInstAlias<"move $dst, $src",
                     (OR GPR32Opnd:$dst, GPR32Opnd:$src, ZERO), 1>,
       GPR_32 {
   let AdditionalPredicates = [NotInMicroMips];
 }

llvm/trunk/test/MC/Mips/macro-seq.s

+# RUN: llvm-mc -arch=mips -mcpu=mips1 < %s | FileCheck --check-prefixes=ALL,MIPS32 %s
+# RUN: llvm-mc -arch=mips -mcpu=mips64 < %s | FileCheck --check-prefixes=ALL,MIPS64 %s
+
+# ALL: .text
+seq $2, $11, $0
+# ALL: sltiu $2, $11, 1
+seq $2, $0, $11
+# ALL: sltiu $2, $11, 1
+seq $2, $0, $0
+# ALL: sltiu $2, $zero, 1
+seq $2, $11, $12
+# ALL: xor $2, $11, $12
+# ALL: sltiu $2, $2, 1
+seq $2, $11, 45
+# ALL: xori $2, $11, 45
+seq $2, $12, 0x76666
+# ALL: lui $1, 7
+# ALL: ori $1, $1, 26214
+# ALL: xor $2, $12, $1
+# ALL: sltiu $2, $2, 1
+seq $2, $3
+# ALL: xor $2, $2, $3
+# ALL: sltiu $2, $2, 1
+seq $2, 0x8888
+# ALL: xori $2, $2, 34952
+# ALL: sltiu $2, $2, 1
+seq $2, $3, -1546
+# MIPS32: addiu $2, $3, 1546
+# MIPS64: daddiu $2, $3, 1546
+# ALL: sltiu $2, $2, 1
+seq $2, -7546
+# MIPS32: addiu $2, $2, 7546
+# MIPS64: daddiu $2, $2, 7546
+# ALL: sltiu $2, $2, 1
+seq $4, $5, -66666
+# ALL: lui $1, 65534
+# ALL: ori $1, $1, 64406
+# ALL: xor $4, $5, $1
+# ALL: sltiu $4, $4, 1
+seq $4, $5, -2147483648
+# ALL: lui $1, 32768
+# ALL: xor $4, $5, $1
+# ALL: sltiu $4, $4, 1
+seq $4, -2147483648
+# ALL: lui $1, 32768
+# ALL: xor $4, $4, $1
+# ALL: sltiu $4, $4, 1
+seq $4, $5, 0
+# ALL: sltiu $4, $5, 1
+seq $4, $zero, 1
+# MIPS32: move $4, $zero
+# MIPS64: daddu $4, $zero, $zero