[mips][mips64r6] Add R_MIPS_PC19_S2

[zoran.jovanovic]

Summary

Implemented R_MIPS_PC19_S2 relocation.

[zoran.jovanovic]

Fixed one comment, one error message nad added check for relocation address and symbol value.

[zoran.jovanovic]

Fixed option in test case.

[dsanders]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1961–1981

There should only be one kind of immediate (including symbols and relocations) in the parser. Adding more causes erratic behaviour that depends on the order instructions appear in the matcher table.

Instead of adding additional parsers you should use custom predicates and/or renderers to restrict and/or convert the immediates parsed by the generic parser.

lib/Target/Mips/MipsInstrInfo.td

321

Delete this line. The generic parser should correctly parse constants and expressions.

[zoran.jovanovic]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1961–1981

Current implementation does not have method that parses immediate or symbol as instruction operand, thus it seems appropriate to implement one for pc relative instructions.
Very similar situation is with target operand of branch or jump instructions, where operand can be immediate, symbol or register, and additional parser is introduced there (MipsAsmParser::ParseJumpTarget). I did the same for operands of pc relative instructions.
I do agree that name of the method is not appropriate because both 18 and 19 bit pc relative operands will be parsed with it. I did change the name to ParsePCRelImm in following patch for R_MIPS_PC18_S3 relocation (http://reviews.llvm.org/D3890). I can do that in this patch or change name completely to avoid referencing 'pc relative'.

[dsanders]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1961–1981

Current implementation does not have method that parses immediate or > symbol as instruction operand, thus it seems appropriate to implement > one for pc relative instructions.

Yes it does, it's ParseJumpTarget. 'ParseJumpTarget' is not meant to correspond to 'jumptarget' in the tablegen definitions (although admittedly this influenced the name since ParseImmOrAnyRegisterOrSymbol is rather long). It is meant to accept any input that could be used with the target of any branch-like/jump-like/call-like instruction.

Just to illustrate the problem with having multiple parsers that accept the same input tokens: One obvious bug in ParsePCRelImm is that it will parse $sp as a symbol named '$sp' rather than as a register. If this parser function is called first it will commit the assembly matcher to treating it as a symbol, even when trying to match another instruction that could accept a GPR (MipsOperand::isGPRAsmReg() will return false because the operand is a k_Immediate).

Even if you fixed that bug by adding the missing ParseAnyRegister call to ParsePCRelImm, having two identical functions will eventually lead to them diverging.

[zoran.jovanovic]

New implementation which does not introduce new parser method.

[dsanders]

Thanks. LGTM

[zoran.jovanovic]

Closed by commit rL210773 (authored by zjovanovic).

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

   MipsAsmParser::OperandMatchResultTy
   ParseLSAImm(SmallVectorImpl<MCParsedAsmOperand *> &Operands);
 
+  MipsAsmParser::OperandMatchResultTy
+  ParsePCRelImm(SmallVectorImpl<MCParsedAsmOperand *> &Operands);
+
   bool searchSymbolAlias(SmallVectorImpl<MCParsedAsmOperand *> &Operands);
 
   bool ParseOperand(SmallVectorImpl<MCParsedAsmOperand *> &,

   return MatchOperand_Success;
 }
 
+MipsAsmParser::OperandMatchResultTy MipsAsmParser::ParsePCRelImm(
+    SmallVectorImpl<MCParsedAsmOperand *> &Operands) {
+  DEBUG(dbgs() << "ParsePCRelImm\n");
+
+  SMLoc S = getLexer().getLoc();
+
+  // Integers and expressions are acceptable
+  OperandMatchResultTy ResTy = ParseImm(Operands);
+  if (ResTy != MatchOperand_NoMatch)
+    return ResTy;
+
+  const MCExpr *Expr = nullptr;
+  if (Parser.parseExpression(Expr)) {
+    // We have no way of knowing if a symbol was consumed so we must ParseFail
+    return MatchOperand_ParseFail;
+  }
+  Operands.push_back(
+      MipsOperand::CreateImm(Expr, S, getLexer().getLoc(), *this));
+  return MatchOperand_Success;
+}
+
 MCSymbolRefExpr::VariantKind MipsAsmParser::getVariantKind(StringRef Symbol) {
 
   MCSymbolRefExpr::VariantKind VK =

lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp

     if (!isIntN(16, Value) && Ctx)
       Ctx->FatalError(Fixup.getLoc(), "out of range PC16 fixup");
     break;
+  case Mips::fixup_Mips_PC19_S2:
+    // Forcing a signed division because Value can be negative.
+    Value = (int64_t)Value / 4;
+    // We now check if Value can be encoded as a 19-bit signed immediate.
+    if (!isIntN(19, Value) && Ctx)
+      Ctx->FatalError(Fixup.getLoc(), "out of range PC19 fixup");
+    break;
   }
 
   return Value;

     { "fixup_Mips_GOT_LO16",     0,     16,   0 },
     { "fixup_Mips_CALL_HI16",    0,     16,   0 },
     { "fixup_Mips_CALL_LO16",    0,     16,   0 },
+    { "fixup_Mips_PC19_S2",      0,     19,   MCFixupKindInfo::FKF_IsPCRel },
     { "fixup_MICROMIPS_26_S1",   0,     26,   0 },
     { "fixup_MICROMIPS_HI16",    0,     16,   0 },
     { "fixup_MICROMIPS_LO16",    0,     16,   0 },

lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp

   case Mips::fixup_MICROMIPS_TLS_TPREL_LO16:
     Type = ELF::R_MICROMIPS_TLS_TPREL_LO16;
     break;
+  case Mips::fixup_Mips_PC19_S2:
+    Type = ELF::R_MIPS_PC19_S2;
+    break;
   }
   return Type;
 }

lib/Target/Mips/MCTargetDesc/MipsFixupKinds.h

     // resulting in - R_MIPS_CALL_LO16
     fixup_Mips_CALL_LO16,
 
+    // resulting in - R_MIPS_PC19_S2
+    fixup_Mips_PC19_S2,
+
     // resulting in - R_MICROMIPS_26_S1
     fixup_MICROMIPS_26_S1,
 

lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp

 MipsMCCodeEmitter::getSimm19Lsl2Encoding(const MCInst &MI, unsigned OpNo,
                                          SmallVectorImpl<MCFixup> &Fixups,
                                          const MCSubtargetInfo &STI) const {
-  assert(MI.getOperand(OpNo).isImm());
-  // The immediate is encoded as 'immediate << 2'.
-  unsigned Res = getMachineOpValue(MI, MI.getOperand(OpNo), Fixups, STI);
-  assert((Res & 3) == 0);
-  return Res >> 2;
+  const MCOperand &MO = MI.getOperand(OpNo);
+  if (MO.isImm()) {
+    // The immediate is encoded as 'immediate << 2'.
+    unsigned Res = getMachineOpValue(MI, MO, Fixups, STI);
+    assert((Res & 3) == 0);
+    return Res >> 2;
+  }
+
+  assert(MO.isExpr() &&
+         "getSimm19Lsl2Encoding expects only expressions or an immediate");
+
+  const MCExpr *Expr = MO.getExpr();
+  Fixups.push_back(MCFixup::Create(0, Expr,
+                                   MCFixupKind(Mips::fixup_Mips_PC19_S2)));
+  return 0;
 }
 
 #include "MipsGenMCCodeEmitter.inc"

lib/Target/Mips/MipsInstrInfo.td

   let DecoderMethod= "DecodeSimm16";
 }
 
+def MipsPCRelImmOperand : AsmOperandClass {
+  let Name = "PCRelImm";
+  let ParserMethod = "ParsePCRelImm";
+  let PredicateMethod = "isImm";
+  let RenderMethod = "addImmOperands";
+}
+
 def simm19_lsl2 : Operand<i32> {
   let EncoderMethod = "getSimm19Lsl2Encoding";
   let DecoderMethod = "DecodeSimm19Lsl2";
+  let ParserMatchClass = MipsPCRelImmOperand;
 }
 
 def simm20      : Operand<i32> {

test/MC/Mips/mips32r6/relocations.s

+# RUN: llvm-mc %s -triple=mipsel-unknown-linux -show-encoding \
+# RUN: -mcpu=mips32r6 | FileCheck %s -check-prefix=CHECK-FIXUP
+# RUN: llvm-mc %s -filetype=obj -triple=mipsel-unknown-linux \
+# RUN: -mcpu=mips32r6 | llvm-readobj -r \
+# RUN: | FileCheck %s -check-prefix=CHECK-ELF
+#------------------------------------------------------------------------------
+# Check that the assembler can handle the documented syntax
+# for relocations.
+#------------------------------------------------------------------------------
+# CHECK-FIXUP: addiupc $2, bar               # encoding: [A,A,0b01000AAA,0xec]
+# CHECK-FIXUP:                               # fixup A - offset: 0,
+# CHECK-FIXUP:                                 value: bar,
+# CHECK-FIXUP:                                 kind: fixup_Mips_PC19_S2
+# CHECK-FIXUP: lwpc    $2, bar               # encoding: [A,A,0b01001AAA,0xec]
+# CHECK-FIXUP:                               #   fixup A - offset: 0,
+# CHECK-FIXUP:                                value: bar,
+# CHECK-FIXUP:                                kind: fixup_Mips_PC19_S2
+# CHECK-FIXUP: lwupc   $2, bar               # encoding: [A,A,0b01010AAA,0xec]
+# CHECK-FIXUP:                               #   fixup A - offset: 0,
+# CHECK-FIXUP:                                value: bar,
+# CHECK-FIXUP:                                kind: fixup_Mips_PC19_S2
+#------------------------------------------------------------------------------
+# Check that the appropriate relocations were created.
+#------------------------------------------------------------------------------
+# CHECK-ELF: Relocations [
+# CHECK-ELF:     0x0 R_MIPS_PC19_S2 bar 0x0
+# CHECK-ELF:     0x4 R_MIPS_PC19_S2 bar 0x0
+# CHECK-ELF:     0x8 R_MIPS_PC19_S2 bar 0x0
+# CHECK-ELF: ]
+
+    addiupc   $2,bar
+    lwpc      $2,bar
+    lwupc     $2,bar
+

test/MC/Mips/mips64r6/relocations.s

+# RUN: llvm-mc %s -triple=mipsel-unknown-linux -show-encoding \
+# RUN: -mcpu=mips32r6 | FileCheck %s -check-prefix=CHECK-FIXUP
+# RUN: llvm-mc %s -filetype=obj -triple=mipsel-unknown-linux \
+# RUN: -mcpu=mips32r6 | llvm-readobj -r \
+# RUN: | FileCheck %s -check-prefix=CHECK-ELF
+#------------------------------------------------------------------------------
+# Check that the assembler can handle the documented syntax
+# for relocations.
+#------------------------------------------------------------------------------
+# CHECK-FIXUP: addiupc $2, bar               # encoding: [A,A,0b01000AAA,0xec]
+# CHECK-FIXUP:                               # fixup A - offset: 0,
+# CHECK-FIXUP:                                 value: bar,
+# CHECK-FIXUP:                                 kind: fixup_Mips_PC19_S2
+# CHECK-FIXUP: lwpc    $2, bar               # encoding: [A,A,0b01001AAA,0xec]
+# CHECK-FIXUP:                               #   fixup A - offset: 0,
+# CHECK-FIXUP:                                value: bar,
+# CHECK-FIXUP:                                kind: fixup_Mips_PC19_S2
+# CHECK-FIXUP: lwupc   $2, bar               # encoding: [A,A,0b01010AAA,0xec]
+# CHECK-FIXUP:                               #   fixup A - offset: 0,
+# CHECK-FIXUP:                                value: bar,
+# CHECK-FIXUP:                                kind: fixup_Mips_PC19_S2
+#------------------------------------------------------------------------------
+# Check that the appropriate relocations were created.
+#------------------------------------------------------------------------------
+# CHECK-ELF: Relocations [
+# CHECK-ELF:     0x0 R_MIPS_PC19_S2 bar 0x0
+# CHECK-ELF:     0x4 R_MIPS_PC19_S2 bar 0x0
+# CHECK-ELF:     0x8 R_MIPS_PC19_S2 bar 0x0
+# CHECK-ELF: ]
+
+    addiupc   $2,bar
+    lwpc      $2,bar
+    lwupc     $2,bar
+