[RISCV] Support linker relax function call from auipc and jalr to jal

[shiva0217]

Summary

To do this:

1. Add fixup_riscv_relax fixup types which eventually will transfer to R_RISCV_RELAX relocation type.

2. Insert R_RISCV_RELAX relocation type to auipc function call expression when linker relaxation enabled.

[apazos]

test/CodeGen/RISCV/linker-relaxation.ll

1

if you decide to add the no-relax flag, maybe you can add the test case for that too.

[shiva0217]

Update patch to address Ana's comments.

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h

50

The description indicates you'd only want to use the call fixup when relaxation is enabled. But there's nothing stopping us from using fixup_riscv_call even when linker relaxation is disabled - it reduces the number of relocations for the linker to process.

How about something like the following, which better explains its use "A fixup representing a call attached to the auipc instruction of an adjacent auipc+pair."

52

Maybe something like "Used to generate an R_RISCV_RELAX relocation type, which indicates the linker may relax the instruction pair."

test/CodeGen/RISCV/linker-relaxation.ll

9

I think we need to test the assembly output here, which will show that %pcrel_lo/%pcrel_hi are still emitted (and also checks that the asm emitter code doesn't crash when encountering VK_RISCV_CALL_LO). It's obviously unfortunate that generating .s and then assembling that will produce a different result (two relocations) than emitting the .o directly. Emitting the 'call` pseudoinstruction as used by gcc would fix that issue, but still leaves you with no way of expressing the "expanded" auipc+jalr form.

[asb]

Could you please add tests that demonstrate the new logic in shouldForceRelocation? Specifically, it would be good to show that relocations are emitted even for local control flow, when they weren't before.

[shiva0217]

Update patch to address Alex's comments.

[shiva0217]

test/CodeGen/RISCV/linker-relaxation.ll

9

Hi Alex. Should we emit the call pseudo instruction as gcc to fix the issue? To do that, we could delay the function call expansion to encoding to preserve call.

[asb]

This seems like a sensible intermediate step, even if it's not a 100% match for gcc behaviour.

I note that assembling asm generated by linker-relaxation.ll results in a RISCV_RELAX being generated with gas attached to both the pcrel_hi20 and the pcrel_lo12_i. I think we definitely want to do the same thing and it probably makes sense to do so before switching to directly generating the RISCV_CALL relocation - do you think?

test/CodeGen/RISCV/linker-relaxation.ll

9

I think in the future we'll want to emit the call pseudoinstruction unless -mexplicit-relocs is set. gcc (at least the version I have built) still seems to emit the call pseudoinstruction when -mexplicit-relocs is set, which seems incorrect to me (though maybe I'm misunderstanding that compiler option).

[asb]

Given that Simon's D45181 will be dependent on this (as it should really query whether linker relaxation is enabled), it probably makes sense to get this merged sooner rather than later.

I think the options are:

1. Merge as is
2. Modify this patch so it adds RISCV_RELAX to pcrel_hi and pcrel_lo relocations when linker relaxation is enabled, then have a follow-up patch that emits CALL.

The psabi doc doesn't discuss attaching RISCV_RELAX to relocations other than RISCV_CALL or RISCV_CALL_PLT, but presumably binutils ld will use it for other pairs?

[shiva0217]

Update patch to reflect the updated in https://reviews.llvm.org/D44885.

[shiva0217]

In D44886#1065531, @asb wrote:

Given that Simon's D45181 will be dependent on this (as it should really query whether linker relaxation is enabled), it probably makes sense to get this merged sooner rather than later.

Hi Alex.

I think the options are:

1. Merge as is

If we merge as is will leave the relocation difference between assembly code and object file as you described.

2. Modify this patch so it adds RISCV_RELAX to pcrel_hi and pcrel_lo relocations when linker relaxation is enabled, then have a follow-up patch that emits CALL.

The psabi doc doesn't discuss attaching RISCV_RELAX to relocations other than RISCV_CALL or RISCV_CALL_PLT, but presumably binutils ld will use it for other pairs?

The psabi doc didn't mention add RISCV_RELAX to pcrel_hi and pcrel_lo relocations and I have done some experiments. It seems that attaching RISCV_RELAX to pcrel_hi and pcrel_lo relocations can't relax aupic and jalr to jal by Binutils ld. So we have to use R_RISCV_CALL and R_RISCV_RELAX for function call relaxation.

I have updated the patch to generate call pseudoinstruction as your comments in https://reviews.llvm.org/D44886?id=140176#inline-398646.

[shiva0217]

rebase patch to current trunk

[asb]

Hi Shiva. Could you please split out the definition of shouldForceRelocation, enableLinkerRelax and FeatureRelax to a separate patch. That way we can commit that followed by all the patches that rely on querying whether linker relaxation is enabled, and then finally enable it via the RISCVMCCodeEmitter changes in this patch. Now there are a few patches we know we want to commit before linker relaxation is actually safe to use, I think that's probably the best way to structure things.

[shiva0217]

Split ShouldForceRelocation+FeatureRelax into a sperate patch for better structure patches as Alex's suggestion.

[shiva0217]

Add fixup_riscv_call and fixup_riscv_relax to getFixupKindInfo table.

[shiva0217]

update testcase

[asb]

Thanks Shiva, a few minor comments but with those resolved it should be good to go.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

90

Good catch that this wasn't present. Given that riscv_call refers to an auipc+jalr shouldn't this be tagged MCFixupKindInfo::FKF_IsPCRel?

The assert that's in this function isn't sufficient, so to avoid missing these fixups in the future, could you change the Infos line to const static MCFixupKindInfo Infos[] and after the array add a static assert like const static MCFixupKindInfo Infos[RISCV::NumTargetFixupKinds].

Feel free to commit this fix directly, as it doesn't really need to be part of this patch (and even if we reverted this patch for some reason, we'd still want to keep the fixup_riscv_call fix).

91

Shouldn't bits = 0 for relax, given that it's purely a tag?

test/MC/RISCV/linker-relaxation.s

4 ↗

(On Diff #146092)

Might be clearer as NORELAX-RELOC

7 ↗

(On Diff #146092)

Could you please add some riscv64 RUN lines

12 ↗

(On Diff #146092)

We should check NORELAX-RELOC-NOT: R_RISCV_RELAX

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

90

Hi Alex. If we tag fixup_riscv_call with FKF_IsPCRel, Assembler will try to applyFixup when the function and the call site within the same compile unit. So when relaxation disabled, shouldForceRelocation return false, will trigger "Unknown fixup kind!" in adjustFixupValue function. I think we should define as {fixup_riscv_call, 0, 32, 0 } or {fixup_riscv_call", 0, 0, 0 } to indicate that the fixup should never apply. Another solution is define as {fixup_riscv_call, 0, 32, MCFixupKindInfo::FKF_IsPCRel } but always return true in shouldForceRelocation if the fixup is fixup_riscv_call. What do you think?

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

90

Hi Alex. I create a revision D47126 for this with a test case. It would be easier to observe the issue and the RISCV developers could apply the patch before we commit to trunk.

[shiva0217]

Update the patch to address Alex's comments.

[asb]

This looks good to me. I'd say this could land prior to the R_RISCV_ALIGN patch if you rebased it appropriately.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
     //
     // For R_RISCV_PCREL_LO12_I, linker will
     // 1. Find high part instruction(auipc) by .Ltmp3
     // 2. Find symbol(foo) in auipc's relocation type
     // 3. Resolve offset by symbol(foo)
     if (FixupKind == RISCV::fixup_riscv_pcrel_lo12_i ||
         FixupKind == RISCV::fixup_riscv_pcrel_hi20)
       return true;
-    return false;
+    return STI.getFeatureBits()[RISCV::FeatureRelax];
   }
 
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override;
 
   unsigned getNumFixupKinds() const override {
     return RISCV::NumTargetFixupKinds;
const static MCFixupKindInfo Infos[RISCV::NumTargetFixupKinds] = {
       { "fixup_riscv_lo12_s",        0,     32,  0 },
       { "fixup_riscv_pcrel_hi20",   12,     20,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_pcrel_lo12_i", 20,     12,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_pcrel_lo12_s",  0,     32,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_jal",          12,     20,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_branch",        0,     32,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_rvc_jump",      2,     11,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_rvc_branch",    0,     16,  MCFixupKindInfo::FKF_IsPCRel }
     };
 
     if (Kind < FirstTargetFixupKind)
       return MCAsmBackend::getFixupKindInfo(Kind);
 
     assert(unsigned(Kind - FirstTargetFixupKind) < getNumFixupKinds() &&
            "Invalid kind!");
     return Infos[Kind - FirstTargetFixupKind];
   }
 

lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp

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;
+  case RISCV::fixup_riscv_relax:
+    return ELF::R_RISCV_RELAX;
   }
 }
 
 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);
 }

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 - For generate R_RISCV_CALL to relax function call.
+  fixup_riscv_call,
+  // fixup_riscv_relax - For generate R_RISCV_RELAX relax relocation type.
+  fixup_riscv_relax,
 
   // 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

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

RISCVMCCodeEmitter::getImmOpValueAsr1(const MCInst &MI, unsigned OpNo,
   }
 
   return getImmOpValue(MI, OpNo, Fixups, STI);
 }
 
 unsigned RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
                                            SmallVectorImpl<MCFixup> &Fixups,
                                            const MCSubtargetInfo &STI) const {
-
+  bool EnableRelax = STI.getFeatureBits()[RISCV::FeatureRelax];
   const MCOperand &MO = MI.getOperand(OpNo);
 
   MCInstrDesc const &Desc = MCII.get(MI.getOpcode());
   unsigned MIFrm = Desc.TSFlags & RISCVII::InstFormatMask;
 
   // If the destination is an immediate, there is nothing to do
   if (MO.isImm())
     return MO.getImm();
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_LO:
+      if (EnableRelax)
+        return 0;
+      else
+        FixupKind = RISCV::fixup_riscv_pcrel_lo12_i;
+      break;
+    case RISCVMCExpr::VK_RISCV_CALL_HI:
+      if (EnableRelax)
+        FixupKind = RISCV::fixup_riscv_call;
+      else
+        FixupKind = RISCV::fixup_riscv_pcrel_hi20;
+      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) {
       FixupKind = RISCV::fixup_riscv_rvc_jump;
     } else if (MIFrm == RISCVII::InstFormatCB) {
       FixupKind = RISCV::fixup_riscv_rvc_branch;
     }
   }
 
   assert(FixupKind != RISCV::fixup_riscv_invalid && "Unhandled expression!");
 
   Fixups.push_back(
       MCFixup::create(0, Expr, MCFixupKind(FixupKind), MI.getLoc()));
   ++MCNumFixups;
 
+  if (EnableRelax) {
+    if (FixupKind == RISCV::fixup_riscv_call) {
+      Fixups.push_back(
+      MCFixup::create(0, Expr, MCFixupKind(RISCV::fixup_riscv_relax),
+                      MI.getLoc()));
+      ++MCNumFixups;
+    }
+  }
+
   return 0;
 }
 
 #include "RISCVGenMCCodeEmitter.inc"

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_LO,
+    VK_RISCV_CALL_HI,
     VK_RISCV_Invalid
   };
 
 private:
   const MCExpr *Expr;
   const VariantKind Kind;
 
   int64_t evaluateAsInt64(int64_t Value) const;

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

StringRef RISCVMCExpr::getVariantKindName(VariantKind Kind) {
   switch (Kind) {
   default:
     llvm_unreachable("Invalid ELF symbol kind");
   case VK_RISCV_LO:
     return "lo";
   case VK_RISCV_HI:
     return "hi";
   case VK_RISCV_PCREL_LO:
+  case VK_RISCV_CALL_LO:
     return "pcrel_lo";
   case VK_RISCV_PCREL_HI:
+  case VK_RISCV_CALL_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_HI || Kind == VK_RISCV_CALL_LO)
     return false;
 
   if (!getSubExpr()->evaluateAsRelocatable(Value, nullptr, nullptr))
     return false;
 
   if (!Value.isAbsolute())
     return false;
 

lib/Target/RISCV/RISCV.td

 def IsRV64 : Predicate<"Subtarget->is64Bit()">,
                        AssemblerPredicate<"Feature64Bit">;
 def IsRV32 : Predicate<"!Subtarget->is64Bit()">,
                        AssemblerPredicate<"!Feature64Bit">;
 
 def RV64           : HwMode<"+64bit">;
 def RV32           : HwMode<"-64bit">;
 
+def FeatureRelax
+    : SubtargetFeature<"relax", "EnableLinkerRelax", "true",
+                       "Enable Linker relaxation.">;
+
 //===----------------------------------------------------------------------===//
 // Registers, calling conventions, instruction descriptions.
 //===----------------------------------------------------------------------===//
 
 include "RISCVRegisterInfo.td"
 include "RISCVCallingConv.td"
 include "RISCVInstrInfo.td"
 

lib/Target/RISCV/RISCVAsmPrinter.cpp

void RISCVAsmPrinter::expandFunctionCall(const MachineInstr *MI) {
   else if (Func.isSymbol())
     Sym = GetExternalSymbolSymbol(Func.getSymbolName());
 
   const MCExpr *Expr;
 
   // Create high part expression HiExpr for AUIPC.
   Expr = MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, OutContext);
   const MCExpr *HiExpr =
-    RISCVMCExpr::create(Expr, RISCVMCExpr::VK_RISCV_PCREL_HI, OutContext);
+    RISCVMCExpr::create(Expr, RISCVMCExpr::VK_RISCV_CALL_HI, OutContext);
 
   // Create low part expression LoExpr for JALR.
   auto HiLabel = OutContext.createTempSymbol();
   Expr = MCSymbolRefExpr::create(HiLabel, MCSymbolRefExpr::VK_None, OutContext);
   const MCExpr *LoExpr =
-    RISCVMCExpr::create(Expr, RISCVMCExpr::VK_RISCV_PCREL_LO, OutContext);
+    RISCVMCExpr::create(Expr, RISCVMCExpr::VK_RISCV_CALL_LO, OutContext);
 
   // Emit .LtmpX:
   OutStreamer->EmitLabel(HiLabel);
 
   // Emit AUPIC Ra, %pcrel_hi(Func)
   TmpInst = MCInstBuilder(RISCV::AUIPC)
               .addReg(Ra)
               .addOperand(MCOperand::createExpr(HiExpr));

lib/Target/RISCV/RISCVSubtarget.h

 class RISCVSubtarget : public RISCVGenSubtargetInfo {
   virtual void anchor();
   bool HasStdExtM = false;
   bool HasStdExtA = false;
   bool HasStdExtF = false;
   bool HasStdExtD = false;
   bool HasStdExtC = false;
   bool HasRV64 = false;
+  bool EnableLinkerRelax = false;
   unsigned XLen = 32;
   MVT XLenVT = MVT::i32;
   RISCVFrameLowering FrameLowering;
   RISCVInstrInfo InstrInfo;
   RISCVRegisterInfo RegInfo;
   RISCVTargetLowering TLInfo;
   SelectionDAGTargetInfo TSInfo;
 
const SelectionDAGTargetInfo *getSelectionDAGInfo() const override {
     return &TSInfo;
   }
   bool hasStdExtM() const { return HasStdExtM; }
   bool hasStdExtA() const { return HasStdExtA; }
   bool hasStdExtF() const { return HasStdExtF; }
   bool hasStdExtD() const { return HasStdExtD; }
   bool hasStdExtC() const { return HasStdExtC; }
   bool is64Bit() const { return HasRV64; }
+  bool enableLinkerRelax() const { return EnableLinkerRelax; }
   MVT getXLenVT() const { return XLenVT; }
   unsigned getXLen() const { return XLen; }
 };
 } // End llvm namespace
 
 #endif

test/CodeGen/RISCV/linker-relaxation.ll

+; RUN: llc -mtriple=riscv32 -filetype=obj -mattr=+relax < %s \
+; RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELOC %s
+
+declare i32 @external_function(i32)
+
+define i32 @test_call_external(i32 %a) nounwind {
+; RELOC: R_RISCV_CALL external_function 0x0
+; RELOC: R_RISCV_RELAX external_function 0x0
+  %1 = call i32 @external_function(i32 %a)
+  ret i32 %1
+}