[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 ↗

(On Diff #139755)

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 ↗

(On Diff #140176)

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 ↗

(On Diff #140176)

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

8 ↗

(On Diff #140176)

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

8 ↗

(On Diff #140176)

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

8 ↗

(On Diff #140176)

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

77 ↗

(On Diff #146092)

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).

78 ↗

(On Diff #146092)

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

77 ↗

(On Diff #146092)

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

77 ↗

(On Diff #146092)

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.

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

const static MCFixupKindInfo Infos[RISCV::NumTargetFixupKinds] = {
       { "fixup_riscv_lo12_i",       20,     12,  0 },
       { "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 }
+      { "fixup_riscv_rvc_branch",    0,     16,  MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_riscv_relax",         0,      0,  0 }
     };
 
     if (Kind < FirstTargetFixupKind)
       return MCAsmBackend::getFixupKindInfo(Kind);
 
     assert(unsigned(Kind - FirstTargetFixupKind) < getNumFixupKinds() &&
            "Invalid kind!");
     return Infos[Kind - FirstTargetFixupKind];

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

unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
   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<MCObjectTargetWriter>
 llvm::createRISCVELFObjectWriter(uint8_t OSABI, bool Is64Bit) {
   return llvm::make_unique<RISCVELFObjectWriter>(OSABI, Is64Bit);
 }

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

enum Fixups {
   // 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_relax - Used to generate an R_RISCV_RELAX relocation type,
+  // which indicates the linker may relax the instruction pair.
+  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

llvm/trunk/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();
unsigned RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
   }
 
   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"

llvm/trunk/test/MC/RISCV/linker-relaxation.s

+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX-RELOC %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=-relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX-RELOC %s
+# RUN: llvm-mc -triple riscv32 -mattr=+relax < %s -show-encoding \
+# RUN:     | FileCheck -check-prefix=RELAX-FIXUP %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX-RELOC %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=-relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX-RELOC %s
+# RUN: llvm-mc -triple riscv64 -mattr=+relax < %s -show-encoding \
+# RUN:     | FileCheck -check-prefix=RELAX-FIXUP %s
+
+.long foo
+
+.L1:
+call foo
+# NORELAX-RELOC: R_RISCV_CALL foo 0x0
+# NORELAX-RELOC-NOT: R_RISCV_RELAX
+# RELAX-RELOC: R_RISCV_CALL foo 0x0
+# RELAX-RELOC: R_RISCV_RELAX foo 0x0
+# RELAX-FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_relax
+# RELAX-FIXUP: fixup B - offset: 0, value: foo, kind:
+beq s1, s1, .L1
+# RELAX-RELOC: R_RISCV_BRANCH .L1 0x0
+# RELAX-FIXUP: fixup A - offset: 0, value: .L1, kind: fixup_riscv_branch