[RISCV] Support "call" pseudoinstruction in the MC layer

[shiva0217]

Summary

To do this:

1. Add PseudoCALLIndirct to match indirect function call.
2. Add PseudoCALL to support parsing and print pseudo call in assembly
3. Expand PseudoCALL to the following form with R_RISCV_CALL relocation type while encoding: auipc ra, func jalr ra, ra, 0

If we expand PseudoCALL before emitting assembly, we will see auipc and jalr pair when compiling with -S.
It's hard for assembly parser to parsing this pair and identify it's semantic is function call and then insert R_RISCV_CALL relocation type. Although we could insert R_RISCV_PCREL_HI20 and R_RISCV_PCREL_LO12_I relocation types instead of R_RISCV_CALL. Due to RISCV relocation design, auipc and jalr pair only can relax to jal with R_RISCV_CALL + R_RISCV_RELAX relocation types.

We expand PseudoCALL as late as encoding(RISCVMCCodeEmitter) instead of before emitting assembly(RISCVAsmPrinter) because we want to preserve call pseudoinstruction in assembly code. It's more readable and assembly parser could identify call assembly and insert R_RISCV_CALL relocation type.

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp

26–27 ↗

(On Diff #143221)

I'd phrase this as "Return true if the given relocation must be with a symbol rather than section plus offset".

lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h

51 ↗

(On Diff #143221)

It would be less ambiguous to say "auipc instruction in a pair composed of adjacent auipc+jalr instructions".

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

89 ↗

(On Diff #143221)

clang-format suggests some slightly different indentation within this function.

91 ↗

(On Diff #143221)

By expanding the 'call' at this late stage, there's no opportunity for the instruction compression MC->MC transform (which you might care about with -mno-relax and a fully resolved target). Of course there is no compressed auipc, and you'd have to be rather lucky to end up with jalr ra, ra, 0. A comment to document this limitation might be worthwhile.

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

35–36 ↗

(On Diff #143221)

Is there any way we can test this has the desired effect? I had a quick play around, and it seems quite difficult to trigger this code path, but perhaps I'm missing something obvious.

test/MC/RISCV/function-call.s

8 ↗

(On Diff #143221)

This needs more tests, e.g. call 1234 will trigger an assertion in the current patch.

I think we want test coverage for at least call 1234, calll %pcrel_lo(1234), call %pcrel_lo(foo), call %lo(1234), call %pcrel_lo(foo).

[shiva0217]

Update patch to address Alex's comments.

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

35–36 ↗

(On Diff #143221)

Hi Alex. The code path can be trigger by llvm-mc -show-encoding and I have added the testing in function-call.s.

test/MC/RISCV/function-call.s

8 ↗

(On Diff #143221)

Hi Alex. I have added the new predicate call_target and relative parsing functions to verify call operand. Thanks for your comments to cover this.

[asb]

Thanks for the updates Shiva. I've added a few more suggested changes, and I think we're almost there.

I think the patch description also needs to be updated to justify why the CALL->AUIPC+JALR instruction should be as late as RISCVMCCodeEmitter and can't be earlier (e.g. RISCVAsmPrinter).

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

174–186 ↗

(On Diff #143688)

When possible, the backend tries to use generic names for operand types and predicates. e.g. isSimm21. Lets make this isBareSymbol. this can be implemented as below:

bool isBareSymbol() const {
  int64_t Imm;
  RISCVMCExpr::VariantKind VK;
  // Must be of 'immediate' type but not a constant.
  if (!isImm() || evaluateConstantImm(Imm, VK))
    return false;
  return RISCVAsmParser::classifySymbolRef(getImm(), VK, Imm) &&
         VK == RISCVMCExpr::VK_RISCV_None;
}

495–499 ↗

(On Diff #143688)

Duplicates addImmOperands. Delete.

725–730 ↗

(On Diff #143688)

case Match_InvalidBareSymbol: {
  SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
  return Error(ErrorLoc, "operand must be a bare symbol name");
}

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

89 ↗

(On Diff #143688)

So -> meaning

90 ↗

(On Diff #143688)

"It's ok because" -> "This is acceptable because".

103 ↗

(On Diff #143688)

assert(Func.isExpr() && "Expected expression");

lib/Target/RISCV/RISCVInstrInfo.td

156–169 ↗

(On Diff #143688)

def BareSymbol : AsmOperandClass {
  let Name = "BareSymbol";
  let RenderMethod = "addImmOperands";
  let DiagnosticType = "InvalidBareSymbol";
}

// A bare symbol.
def bare_symbol : Operand<XLenVT> {
  let ParserMatchClass = BareSymbol;
  let MCOperandPredicate = [{
     return MCOp.isBareSymbolRef();
  }];
}

[shiva0217]

Update patch to address Alex's comments.

[asb]

Thanks Shiva, this is looking good to me.

In the process of committing, it's probably worth adding a call bar to function-call.s with the same set of checks as for call foo just so there's coverage for a call to an external symbol.

llvm/trunk/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

if (!IsConstantImm)
       IsValid = RISCVAsmParser::classifySymbolRef(getImm(), VK, Imm);
     else
       IsValid = isShiftedInt<N - 1, 1>(Imm);
     return IsValid && VK == RISCVMCExpr::VK_RISCV_None;
   }
 
   // Predicate methods for AsmOperands defined in RISCVInstrInfo.td
 
+  bool isBareSymbol() const {
+    int64_t Imm;
+    RISCVMCExpr::VariantKind VK;
+    // Must be of 'immediate' type but not a constant.
+    if (!isImm() || evaluateConstantImm(Imm, VK))
+      return false;
+    return RISCVAsmParser::classifySymbolRef(getImm(), VK, Imm) &&
+           VK == RISCVMCExpr::VK_RISCV_None;
+  }
+
   /// Return true if the operand is a valid for the fence instruction e.g.
   /// ('iorw').
   bool isFenceArg() const {
     if (!isImm())
       return false;
     const MCExpr *Val = getImm();
     auto *SVal = dyn_cast<MCSymbolRefExpr>(Val);
     if (!SVal || SVal->getKind() != MCSymbolRefExpr::VK_None)
return Error(
         "operand must be formed of letters selected in-order from 'iorw'");
   }
   case Match_InvalidFRMArg: {
     SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
     return Error(
         ErrorLoc,
         "operand must be a valid floating point rounding mode mnemonic");
   }
+  case Match_InvalidBareSymbol: {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc, "operand must be a bare symbol name");
+  }
   }
 
   llvm_unreachable("Unknown match type detected!");
 }
 
 bool RISCVAsmParser::ParseRegister(unsigned &RegNo, SMLoc &StartLoc,
                                    SMLoc &EndLoc) {
   const AsmToken &Tok = getParser().getTok();

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp

 
 namespace {
 class RISCVELFObjectWriter : public MCELFObjectTargetWriter {
 public:
   RISCVELFObjectWriter(uint8_t OSABI, bool Is64Bit);
 
   ~RISCVELFObjectWriter() override;
 
+  // Return true if the given relocation must be with a symbol rather than
+  // section plus offset.
+  bool needsRelocateWithSymbol(const MCSymbol &Sym,
+                               unsigned Type) const override {
+    // TODO: this is very conservative, update once RISC-V psABI requirements
+    //       are clarified.
+    return true;
+  }
+
 protected:
   unsigned getRelocType(MCContext &Ctx, const MCValue &Target,
                         const MCFixup &Fixup, bool IsPCRel) const override;
 };
 }
 
 RISCVELFObjectWriter::RISCVELFObjectWriter(uint8_t OSABI, bool Is64Bit)
     : MCELFObjectTargetWriter(Is64Bit, OSABI, ELF::EM_RISCV,
unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
   case RISCV::fixup_riscv_jal:
     return ELF::R_RISCV_JAL;
   case RISCV::fixup_riscv_branch:
     return ELF::R_RISCV_BRANCH;
   case RISCV::fixup_riscv_rvc_jump:
     return ELF::R_RISCV_RVC_JUMP;
   case RISCV::fixup_riscv_rvc_branch:
     return ELF::R_RISCV_RVC_BRANCH;
+  case RISCV::fixup_riscv_call:
+    return ELF::R_RISCV_CALL;
   }
 }
 
 std::unique_ptr<MCObjectWriter>
 llvm::createRISCVELFObjectWriter(raw_pwrite_stream &OS, uint8_t OSABI,
                                  bool Is64Bit) {
   return createELFObjectWriter(
       llvm::make_unique<RISCVELFObjectWriter>(OSABI, Is64Bit), OS,
       /*IsLittleEndian=*/true);
 }

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h

enum Fixups {
   // instructions
   fixup_riscv_branch,
   // fixup_riscv_rvc_jump - 11-bit fixup for symbol references in the
   // compressed jump instruction
   fixup_riscv_rvc_jump,
   // fixup_riscv_rvc_branch - 8-bit fixup for symbol references in the
   // compressed branch instruction
   fixup_riscv_rvc_branch,
+  // fixup_riscv_call - A fixup representing a call attached to the auipc
+  // instruction in a pair composed of adjacent auipc+jalr instructions.
+  fixup_riscv_call,
 
   // fixup_riscv_invalid - used as a sentinel and a marker, must be last fixup
   fixup_riscv_invalid,
   NumTargetFixupKinds = fixup_riscv_invalid - FirstTargetFixupKind
 };
 } // end namespace RISCV
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

 #include "MCTargetDesc/RISCVMCExpr.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
RISCVMCCodeEmitter(MCContext &ctx, MCInstrInfo const &MCII)
       : Ctx(ctx), MCII(MCII) {}
 
   ~RISCVMCCodeEmitter() override {}
 
   void encodeInstruction(const MCInst &MI, raw_ostream &OS,
                          SmallVectorImpl<MCFixup> &Fixups,
                          const MCSubtargetInfo &STI) const override;
 
+  void expandFunctionCall(const MCInst &MI, raw_ostream &OS,
+                          SmallVectorImpl<MCFixup> &Fixups,
+                          const MCSubtargetInfo &STI) const;
+
   /// TableGen'erated function for getting the binary encoding for an
   /// instruction.
   uint64_t getBinaryCodeForInstr(const MCInst &MI,
                                  SmallVectorImpl<MCFixup> &Fixups,
                                  const MCSubtargetInfo &STI) const;
 
   /// Return binary encoding of operand. If the machine operand requires
   /// relocation, record the relocation and return zero.

 } // end anonymous namespace
 
 MCCodeEmitter *llvm::createRISCVMCCodeEmitter(const MCInstrInfo &MCII,
                                               const MCRegisterInfo &MRI,
                                               MCContext &Ctx) {
   return new RISCVMCCodeEmitter(Ctx, MCII);
 }
 
+// Expand PseudoCALL to AUIPC and JALR with relocation types.
+// We expand PseudoCALL while encoding, meaning AUIPC and JALR won't go through
+// RISCV MC to MC compressed instruction transformation. This is acceptable
+// because AUIPC has no 16-bit form and C_JALR have no immediate operand field.
+// We let linker relaxation deal with it. When linker relaxation enabled,
+// AUIPC and JALR have chance relax to JAL. If C extension is enabled,
+// JAL has chance relax to C_JAL.
+void RISCVMCCodeEmitter::expandFunctionCall(const MCInst &MI, raw_ostream &OS,
+                                            SmallVectorImpl<MCFixup> &Fixups,
+                                            const MCSubtargetInfo &STI) const {
+  MCInst TmpInst;
+  MCOperand Func = MI.getOperand(0);
+  unsigned Ra = RISCV::X1;
+  uint32_t Binary;
+
+  assert(Func.isExpr() && "Expected expression");
+
+  const MCExpr *Expr = Func.getExpr();
+
+  // Create function call expression CallExpr for AUIPC.
+  const MCExpr *CallExpr =
+      RISCVMCExpr::create(Expr, RISCVMCExpr::VK_RISCV_CALL, Ctx);
+
+  // Emit AUIPC Ra, Func with R_RISCV_CALL relocation type.
+  TmpInst = MCInstBuilder(RISCV::AUIPC)
+                .addReg(Ra)
+                .addOperand(MCOperand::createExpr(CallExpr));
+  Binary = getBinaryCodeForInstr(TmpInst, Fixups, STI);
+  support::endian::Writer<support::little>(OS).write(Binary);
+
+  // Emit JALR Ra, Ra, 0
+  TmpInst = MCInstBuilder(RISCV::JALR).addReg(Ra).addReg(Ra).addImm(0);
+  Binary = getBinaryCodeForInstr(TmpInst, Fixups, STI);
+  support::endian::Writer<support::little>(OS).write(Binary);
+}
+
 void RISCVMCCodeEmitter::encodeInstruction(const MCInst &MI, raw_ostream &OS,
                                            SmallVectorImpl<MCFixup> &Fixups,
                                            const MCSubtargetInfo &STI) const {
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
   // Get byte count of instruction.
   unsigned Size = Desc.getSize();
 
+  if (MI.getOpcode() == RISCV::PseudoCALL) {
+    expandFunctionCall(MI, OS, Fixups, STI);
+    MCNumEmitted += 2;
+    return;
+  }
+
   switch (Size) {
   default:
     llvm_unreachable("Unhandled encodeInstruction length!");
   case 2: {
     uint16_t Bits = getBinaryCodeForInstr(MI, Fixups, STI);
     support::endian::Writer<support::little>(OS).write<uint16_t>(Bits);
     break;
   }
case RISCVMCExpr::VK_RISCV_PCREL_LO:
         FixupKind = RISCV::fixup_riscv_pcrel_lo12_s;
       else
         llvm_unreachable(
             "VK_RISCV_PCREL_LO used with unexpected instruction format");
       break;
     case RISCVMCExpr::VK_RISCV_PCREL_HI:
       FixupKind = RISCV::fixup_riscv_pcrel_hi20;
       break;
+    case RISCVMCExpr::VK_RISCV_CALL:
+      FixupKind = RISCV::fixup_riscv_call;
+      break;
     }
   } else if (Kind == MCExpr::SymbolRef &&
              cast<MCSymbolRefExpr>(Expr)->getKind() == MCSymbolRefExpr::VK_None) {
     if (Desc.getOpcode() == RISCV::JAL) {
       FixupKind = RISCV::fixup_riscv_jal;
     } else if (MIFrm == RISCVII::InstFormatB) {
       FixupKind = RISCV::fixup_riscv_branch;
     } else if (MIFrm == RISCVII::InstFormatCJ) {

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.h

 class RISCVMCExpr : public MCTargetExpr {
 public:
   enum VariantKind {
     VK_RISCV_None,
     VK_RISCV_LO,
     VK_RISCV_HI,
     VK_RISCV_PCREL_LO,
     VK_RISCV_PCREL_HI,
+    VK_RISCV_CALL,
     VK_RISCV_Invalid
   };
 
 private:
   const MCExpr *Expr;
   const VariantKind Kind;
 
   int64_t evaluateAsInt64(int64_t Value) const;

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

 #define DEBUG_TYPE "riscvmcexpr"
 
 const RISCVMCExpr *RISCVMCExpr::create(const MCExpr *Expr, VariantKind Kind,
                                        MCContext &Ctx) {
   return new (Ctx) RISCVMCExpr(Expr, Kind);
 }
 
 void RISCVMCExpr::printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const {
-  bool HasVariant = getKind() != VK_RISCV_None;
+  bool HasVariant =
+      ((getKind() != VK_RISCV_None) && (getKind() != VK_RISCV_CALL));
   if (HasVariant)
     OS << '%' << getVariantKindName(getKind()) << '(';
   Expr->print(OS, MAI);
   if (HasVariant)
     OS << ')';
 }
 
 bool RISCVMCExpr::evaluateAsRelocatableImpl(MCValue &Res,
StringRef RISCVMCExpr::getVariantKindName(VariantKind Kind) {
   case VK_RISCV_PCREL_HI:
     return "pcrel_hi";
   }
 }
 
 bool RISCVMCExpr::evaluateAsConstant(int64_t &Res) const {
   MCValue Value;
 
-  if (Kind == VK_RISCV_PCREL_HI || Kind == VK_RISCV_PCREL_LO)
+  if (Kind == VK_RISCV_PCREL_HI || Kind == VK_RISCV_PCREL_LO ||
+      Kind == VK_RISCV_CALL)
     return false;
 
   if (!getSubExpr()->evaluateAsRelocatable(Value, nullptr, nullptr))
     return false;
 
   if (!Value.isAbsolute())
     return false;
 

llvm/trunk/lib/Target/RISCV/RISCVInstrInfo.td

def simm21_lsb0 : Operand<OtherVT> {
   let MCOperandPredicate = [{
     int64_t Imm;
     if (MCOp.evaluateAsConstantImm(Imm))
       return isShiftedInt<20, 1>(Imm);
     return MCOp.isBareSymbolRef();
   }];
 }
 
+def BareSymbol : AsmOperandClass {
+  let Name = "BareSymbol";
+  let RenderMethod = "addImmOperands";
+  let DiagnosticType = "InvalidBareSymbol";
+}
+
+// A bare symbol.
+def bare_symbol : Operand<XLenVT> {
+  let ParserMatchClass = BareSymbol;
+  let MCOperandPredicate = [{
+     return MCOp.isBareSymbolRef();
+  }];
+}
+
 // A parameterized register class alternative to i32imm/i64imm from Target.td.
 def ixlenimm : Operand<XLenVT>;
 
 // Standalone (codegen-only) immleaf patterns.
 def simm32     : ImmLeaf<XLenVT, [{return isInt<32>(Imm);}]>;
 def simm32hi20 : ImmLeaf<XLenVT, [{return isShiftedInt<20, 12>(Imm);}]>;
 
 // Addressing modes.

 let isBarrier = 1, isBranch = 1, isIndirectBranch = 1, isTerminator = 1 in
 def PseudoBRIND : Pseudo<(outs), (ins GPR:$rs1, simm12:$imm12), []>,
                   PseudoInstExpansion<(JALR X0, GPR:$rs1, simm12:$imm12)>;
 
 def : Pat<(brind GPR:$rs1), (PseudoBRIND GPR:$rs1, 0)>;
 def : Pat<(brind (add GPR:$rs1, simm12:$imm12)),
           (PseudoBRIND GPR:$rs1, simm12:$imm12)>;
 
+// PseudoCALL is a pseudo instruction which will eventually expand to auipc
+// and jalr. Define AsmString because we want assembler could print "call"
+// when compile with -S. Define isCodeGenOnly = 0 because we want parser
+// could parsing assembly "call" instruction.
+let isCall = 1, Defs = [X1], isCodeGenOnly = 0,
+    hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+def PseudoCALL : Pseudo<(outs), (ins bare_symbol:$func),
+                        []> {
+  let AsmString = "call\t$func";
+}
+
 let isCall = 1, Defs = [X1] in
-def PseudoCALL : Pseudo<(outs), (ins GPR:$rs1), [(Call GPR:$rs1)]>,
+def PseudoCALLIndirect : Pseudo<(outs), (ins GPR:$rs1), [(Call GPR:$rs1)]>,
                          PseudoInstExpansion<(JALR X1, GPR:$rs1, 0)>;
 
 let isBarrier = 1, isReturn = 1, isTerminator = 1 in
 def PseudoRET : Pseudo<(outs), (ins), [(RetFlag)]>,
                 PseudoInstExpansion<(JALR X0, X1, 0)>;
 
 /// Loads
 
 multiclass LdPat<PatFrag LoadOp, RVInst Inst> {

llvm/trunk/test/MC/RISCV/function-call-invalid.s

+# RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
+
+call 1234 # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %pcrel_hi(1234) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %pcrel_lo(1234) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %pcrel_hi(foo) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %pcrel_lo(foo) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %hi(1234) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %lo(1234) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %hi(foo) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name
+call %lo(foo) # CHECK: :[[@LINE]]:6: error: operand must be a bare symbol name

llvm/trunk/test/MC/RISCV/function-call.s

+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELOC %s
+# RUN: llvm-mc -triple riscv32 < %s -show-encoding \
+# RUN:     | FileCheck -check-prefix=FIXUP %s
+
+.long foo
+
+call foo
+# RELOC: R_RISCV_CALL foo 0x0
+# INSTR: auipc ra, 0
+# INSTR: jalr  ra
+# FIXUP: fixup A - offset: 0, value: foo, kind:
+call bar
+# RELOC: R_RISCV_CALL bar 0x0
+# INSTR: auipc ra, 0
+# INSTR: jalr  ra
+# FIXUP: fixup A - offset: 0, value: bar, kind: