[RISCV] Implement MC layer support for the tail pseudoinstruction

[mgrang]

Summary

This patch implements MC support for tail psuedo instruction.
A follow-up patch implements the codegen support as well as handling of the indirect tail pseudo instruction.

[mgrang]

Here's the patch to generate tail pseudo instruction: https://reviews.llvm.org/D45395

[asb]

This patch would be better named 'Implement MC layer support for the tail pseudoinstruction' or similar. It should apply to the current head ,with D45395 rebased to apply on top of it. This is because MC layer support is a useful incremental change, and can be cleanly separated from the codegen changes (meaning we can revert one without affecting the other).

It looks like you'd just need to add the PseudoTAIL definition to RISCVInstrInfo.td and remove the PseudoTAILIndirect reference from this patch (it can be introduced in D45935).

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

134

This line is probably going to be deleted in this patch anyway, but at least for the follow-up patch that includes PseudoTAILIndirect it might be better to have expandFunctionCall return the number of MIs inserted.

[mgrang]

Thanks for your comments @asb. There seems to be some problem with long double function arguments which I wanted to clarify.

The RISCV Calling Convention (https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf) says that for soft float abi, long doubles are passed in registers via reference:

In RV32, for example, the function double foo(int, double, long double) is passed its first argument in a0, its second argument in a2 and a3, and its third argument by reference via a4

However, consider this code:

__attribute__((noinline))
int bar(long double a) {
  printf("### Val L: %LF\n", a);
  return a;
}

__attribute__((noinline))
int foo() {
  long double a = 1;
  return bar(1);
}

Here's the assembly for function foo:

Without tail call:
_Z3foov:                                # @_Z3foov
# %bb.0:                                # %entry
	addi	sp, sp, -32
	sw	ra, 28(sp)
	lui	a0, 262128
	sw	a0, 12(sp)
	sw	zero, 8(sp)
	sw	zero, 4(sp)
	sw	zero, 0(sp)
	mv	a0, sp
	call	_Z3bare
	lw	ra, 28(sp)
	addi	sp, sp, 32
	ret

With tail call:
_Z3foov:                                # @_Z3foov
# %bb.0:                                # %entry
	addi	sp, sp, -16
	lui	a0, 262128
	sw	a0, 12(sp)
	sw	zero, 8(sp)
	sw	zero, 4(sp)
	sw	zero, 0(sp)
	mv	a0, sp
	addi	sp, sp, 16
	tail	_Z3bare

I do not understand why the long double argument is passed via stack. For tail call, this results in an incorrect value being printed inside function bar:

### Val: 0.000000

For double arguments, the assembly looks fine:

_Z3foov:                                # @_Z3foov
# %bb.0:                                # %entry
	lui	a1, 261888
	mv	a0, zero
	tail	_Z3bard

[asb]

These are the possibilities for parameter passing on RV32I/RV64I:

1. Directly in a single register
2. Directly in a pair of registers
3. Directly in a register stack location
4. Indirectly in a register
5. Indirectly in a stack location

Because long double (fp128) and i128 are larger than 2*XLEN, they are passed indirectly and options 1-3 don't apply. Therefore the address of the value will be passed in a register, or if not available then the address is put on the stack. In order to pass indirectly, space on the stack often needs to be allocated in order to store the value. You're seeing this in the example code you pasted. You can see the logic for handling this in RISCVISelLowering.cpp if you grep for CCValAssign::Indirect.

Thanks for catching this issue. Rejecting tailcall optimisation when CCInfo.getNextStackOffset() != 0 isn't quit enough, we need to also reject it if any of the CCValAssign ArgsLocs are passed CCValAssign::Indirect (I can see the ARM's isEligibleForTailCall does that check too).

[mgrang]

Thanks Alex for the explanation on indirect parameter passing. I have updated the condition in my patch and added a unit test.

[mgrang]

test/CodeGen/RISCV/tail-calls.ll

1 ↗

(On Diff #145062)

Note: I have limited this test only to riscv32. With riscv64 I get the following error for fp128 parameter:
LLVM ERROR: Cannot select: t17: i64 = Constant<4611404543450677248>

I guess I can restrict only the failing test to riscv32 but I felt it was cleaner this way. We could enable this for riscv64 once the support for fp128 in riscv64 is more mature.

@asb What do you suggest?

[mgrang]

test/CodeGen/RISCV/tail-calls.ll

1 ↗

(On Diff #145062)

Please disregard the above comment. It was meant to be added to D45395.

[asb]

Thanks Mandeep, just a few minor cleanups

lib/Target/RISCV/RISCVISelLowering.cpp

1423–1424

This change is needed for codegen rather than MC and should go in D45395

lib/Target/RISCV/RISCVISelLowering.h

32

This change is needed for codegen rather than MC and should go in D45395

lib/Target/RISCV/RISCVInstrInfo.td

41–43

This change is needed for codegen rather than MC and should go in D45395

666

Do we actually need Uses = [X2]?

672–675

This change is needed for codegen rather than MC and should go in D45395

test/MC/RISCV/tail-call-invalid.s

1

Please add a riscv64 RUN line

test/MC/RISCV/tail-call.s

7

Please add riscv64 RUN lines

[mgrang]

Addressed comments.

[mgrang]

lib/Target/RISCV/RISCVInstrInfo.td

666

I decided to align this with ARM which has Uses = [SP] in its definition of Tail.

[mgrang]

lib/Target/RISCV/RISCVInstrInfo.td

667

If I remove the definition for the Tail node then it cannot determine hasSideEfects, mayStore and mayLoad properties:

def Tail         : SDNode<"RISCVISD::TAIL", SDT_RISCVCall,
                          [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue,
                           SDNPVariadic]>;

So I had to explicitly specify them here. We can remove these in the next patch which includes the definition for Tail.

[asb]

Thanks, looks good to me.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

bool RISCVAsmParser::ParseInstruction(ParseInstructionInfo &Info,
   // First operand is token for instruction
   Operands.push_back(RISCVOperand::createToken(Name, NameLoc, isRV64()));
 
   // If there are no more operands, then finish
   if (getLexer().is(AsmToken::EndOfStatement))
     return false;
 
   // Parse first operand
-  if (parseOperand(Operands, Name == "call"))
+  bool ForceImmediate = (Name == "call" || Name == "tail");
+  if (parseOperand(Operands, ForceImmediate))
     return true;
 
   // Parse until end of statement, consuming commas between operands
   while (getLexer().is(AsmToken::Comma)) {
     // Consume comma token
     getLexer().Lex();
 
     // Parse next operand

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

 // 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;
+  unsigned Ra = (MI.getOpcode() == RISCV::PseudoTAIL) ? RISCV::X6 : 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 =

 
 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) {
+  if (MI.getOpcode() == RISCV::PseudoCALL ||
+      MI.getOpcode() == RISCV::PseudoTAIL) {
     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);

lib/Target/RISCV/RISCVISelLowering.h

 class RISCVSubtarget;
 namespace RISCVISD {
 enum NodeType : unsigned {
   FIRST_NUMBER = ISD::BUILTIN_OP_END,
   RET_FLAG,
   CALL,
   SELECT_CC,
   BuildPairF64,
-  SplitF64
+  SplitF64,
+  TAIL
 };
 }
 
 class RISCVTargetLowering : public TargetLowering {
   const RISCVSubtarget &Subtarget;
 
 public:
   explicit RISCVTargetLowering(const TargetMachine &TM,

lib/Target/RISCV/RISCVISelLowering.cpp

const char *RISCVTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case RISCVISD::CALL:
     return "RISCVISD::CALL";
   case RISCVISD::SELECT_CC:
     return "RISCVISD::SELECT_CC";
   case RISCVISD::BuildPairF64:
     return "RISCVISD::BuildPairF64";
   case RISCVISD::SplitF64:
     return "RISCVISD::SplitF64";
+  case RISCVISD::TAIL:
+    return "RISCVISD::TAIL";
   }
   return nullptr;
 }
 
 std::pair<unsigned, const TargetRegisterClass *>
 RISCVTargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                                   StringRef Constraint,
                                                   MVT VT) const {

lib/Target/RISCV/RISCVInstrInfo.td

 def CallSeqStart : SDNode<"ISD::CALLSEQ_START", SDT_RISCVCallSeqStart,
                           [SDNPHasChain, SDNPOutGlue]>;
 def CallSeqEnd   : SDNode<"ISD::CALLSEQ_END", SDT_RISCVCallSeqEnd,
                           [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue]>;
 def RetFlag      : SDNode<"RISCVISD::RET_FLAG", SDTNone,
                           [SDNPHasChain, SDNPOptInGlue, SDNPVariadic]>;
 def SelectCC     : SDNode<"RISCVISD::SELECT_CC", SDT_RISCVSelectCC,
                           [SDNPInGlue]>;
+def Tail         : SDNode<"RISCVISD::TAIL", SDT_RISCVCall,
+                          [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue,
+                           SDNPVariadic]>;
 
 //===----------------------------------------------------------------------===//
 // Operand and SDNode transformation definitions.
 //===----------------------------------------------------------------------===//
 
 class ImmAsmOperand<string prefix, int width, string suffix> : AsmOperandClass {
   let Name = prefix # "Imm" # width # suffix;
   let RenderMethod = "addImmOperands";

 let isCall = 1, Defs = [X1] in
 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)>;
 
+// PseudoTAIL is a pseudo instruction similar to PseudoCALL and will eventually
+// expand to auipc and jalr while encoding.
+// Define AsmString to print "tail" when compile with -S flag.
+let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [X2],
+    isCodeGenOnly = 0 in
+def PseudoTAIL : Pseudo<(outs), (ins bare_symbol:$dst), []> {
+  let AsmString = "tail\t$dst";
+}
+
+def : Pat<(Tail (iPTR tglobaladdr:$dst)),
+          (PseudoTAIL texternalsym:$dst)>;
+def : Pat<(Tail (iPTR texternalsym:$dst)),
+          (PseudoTAIL texternalsym:$dst)>;
+
 /// Loads
 
 multiclass LdPat<PatFrag LoadOp, RVInst Inst> {
   def : Pat<(LoadOp GPR:$rs1), (Inst GPR:$rs1, 0)>;
   def : Pat<(LoadOp AddrFI:$rs1), (Inst AddrFI:$rs1, 0)>;
   def : Pat<(LoadOp (add GPR:$rs1, simm12:$imm12)),
             (Inst GPR:$rs1, simm12:$imm12)>;
   def : Pat<(LoadOp (add AddrFI:$rs1, simm12:$imm12)),

test/MC/RISCV/tail-call-invalid.s

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

test/MC/RISCV/tail-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
+
+tail foo
+# RELOC: R_RISCV_CALL foo 0x0
+# INSTR: auipc t1, 0
+# INSTR: jalr  t1
+# FIXUP: fixup A - offset: 0, value: foo, kind:
+tail bar
+# RELOC: R_RISCV_CALL bar 0x0
+# INSTR: auipc t1, 0
+# INSTR: jalr  t1
+# FIXUP: fixup A - offset: 0, value: bar, kind:
+
+# Ensure that tail calls to functions whose names coincide with register names
+# work.
+
+tail zero
+# RELOC: R_RISCV_CALL zero 0x0
+# INSTR: auipc t1, 0
+# INSTR: jalr  t1
+# FIXUP: fixup A - offset: 0, value: zero, kind:
+
+tail f1
+# RELOC: R_RISCV_CALL f1 0x0
+# INSTR: auipc t1, 0
+# INSTR: jalr  t1
+# FIXUP: fixup A - offset: 0, value: f1, kind:
+
+tail ra
+# RELOC: R_RISCV_CALL ra 0x0
+# INSTR: auipc t1, 0
+# INSTR: jalr  t1
+# FIXUP: fixup A - offset: 0, value: ra, kind: