[RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table

[shiva0217]

Summary

And also adding static_assert after array declaration
to avoid missing any new fixup in MCFixupKindInfo in the future.

Fixup fixup_riscv_call by assembler if the function and callsite within the same compile unit and the linker relaxation disabled.

The patch is according to
https://reviews.llvm.org/D44886#inline-411881

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

57

This comment should explain why we need to preserve the relocation type. e.g. "We are unable to correctly resolve a fixup_riscv_call, so always emit a relocation."

92

Thinking about it, bits should really be 64 for fixup_riscv_call. With that in place, wouldn't it be feasible to add support to adjustFixupValue so we actually can resolve the fixup?

95

You can just use array_lengthof(Infos).

97

"Not all fixup kinds added to Infos array" is probably sufficient.

[asb]

test/CodeGen/RISCV/function-call.ll

3 ↗

(On Diff #147736)

I think a comment explaining the purpose of the test would be useful. Presumably it's something like "Check that not attempt is made to resolve a call fixups and a relocation is always emitted". Of course this would be unnecessary if we could add support for correctly resolving call fixups!

[shiva0217]

Hi Alex.It seems that fixup fixup_riscv_call by assembler is workable. Thanks for your comments.

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

280

It would be helpful to have a comment to explain the addition of 0x800ULL.

test/CodeGen/RISCV/function-call.ll

1–3 ↗

(On Diff #148397)

Now this test has changed to just checking the fixup is resolved when -mattr=-relax I think it makes much more sense as an MC layer test. Perhaps expanding test/MC/RISCV/fixups.s? We need some reassurance that the fixup calculation is correct, even for offsets > 12 bits.

It looks like you have to use .space as in fixups.s, as the following still results in a relocation, even though I would have expected it not to:

.equ addr, 0xdeadbeef
call addr

[asb]

test/CodeGen/RISCV/function-call.ll

1–3 ↗

(On Diff #148397)

Sorry - I didn't turn my brain on for that comment. It's absolutely correct that the call to an absolute target results in a relocation, as of course call is pc-relative. Any way, more tests in test/MC/RISCV/fixups.s to give reassurance that resolvable fixups are encoded correctly would be really welcome.

[shiva0217]

Update test cases to address Alex's comments.

[asb]

Thanks, looks good to me. Probably worth retitling the commit to e.g. "[RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table"

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

public:
   std::unique_ptr<MCObjectTargetWriter>
   createObjectTargetWriter() const override;
 
   // If linker relaxation is enabled, always emit relocations even if the fixup
   // can be resolved. This is necessary for correctness as offsets may change
   // during relaxation.
   bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
                              const MCValue &Target) override {
     return STI.getFeatureBits()[RISCV::FeatureRelax];
   }
 
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override {
     llvm_unreachable("Handled by fixupNeedsRelaxationAdvanced");
   }
 
   bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, bool Resolved,
                                     uint64_t Value,
                                     const MCRelaxableFragment *DF,
                                     const MCAsmLayout &Layout,
                                     const bool WasForced) const override;
 
   unsigned getNumFixupKinds() const override {
     return RISCV::NumTargetFixupKinds;
   }
 
   const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override {
-    const static MCFixupKindInfo Infos[RISCV::NumTargetFixupKinds] = {
+    const static MCFixupKindInfo Infos[] = {
       // This table *must* be in the order that the fixup_* kinds are defined in
       // RISCVFixupKinds.h.
       //
       // name                      offset bits  flags
       { "fixup_riscv_hi20",         12,     20,  0 },
       { "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_call",          0,     64,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_relax",         0,      0,  0 }
     };
+    static_assert((array_lengthof(Infos)) == RISCV::NumTargetFixupKinds,
+                  "Not all fixup kinds added to Infos array");
 
     if (Kind < FirstTargetFixupKind)
       return MCAsmBackend::getFixupKindInfo(Kind);
 
     assert(unsigned(Kind - FirstTargetFixupKind) < getNumFixupKinds() &&
            "Invalid kind!");
     return Infos[Kind - FirstTargetFixupKind];
   }
 
case RISCV::fixup_riscv_branch: {
     unsigned Lo4 = (Value >> 1) & 0xf;
     // Inst{31} = Sbit;
     // Inst{30-25} = Mid6;
     // Inst{11-8} = Lo4;
     // Inst{7} = Hi1;
     Value = (Sbit << 31) | (Mid6 << 25) | (Lo4 << 8) | (Hi1 << 7);
     return Value;
   }
+  case RISCV::fixup_riscv_call: {
+    // Jalr will add UpperImm with the sign-extended 12-bit LowerImm,
+    // we need to add 0x800ULL before extract upper bits to reflect the
+    // effect of the sign extension.
+    uint64_t UpperImm = (Value + 0x800ULL) & 0xfffff000ULL;
+    uint64_t LowerImm = Value & 0xfffULL;
+    return UpperImm | ((LowerImm << 20) << 32);
+  }
   case RISCV::fixup_riscv_rvc_jump: {
     // Need to produce offset[11|4|9:8|10|6|7|3:1|5] from the 11-bit Value.
     unsigned Bit11  = (Value >> 11) & 0x1;
     unsigned Bit4   = (Value >> 4) & 0x1;
     unsigned Bit9_8 = (Value >> 8) & 0x3;
     unsigned Bit10  = (Value >> 10) & 0x1;
     unsigned Bit6   = (Value >> 6) & 0x1;
     unsigned Bit7   = (Value >> 7) & 0x1;

test/MC/RISCV/fixups.s

 
 .fill 329876
 addi zero, zero, 0
 .LBB2:
 
 .set val, 0x12345678
 
 # CHECK-REL-NOT: R_RISCV
+
+# Testing the function call offset could resovled by assembler
+# when the function and the callsite within the same compile unit
+# and the linker relaxation is disabled.
+func:
+.fill 100
+call func
+# CHECK-FIXUP: fixup A - offset: 0, value: func, kind: fixup_riscv_call
+# CHECK-INSTR: auipc   ra, 0
+# CHECK-INSTR: jalr    ra, ra, -100
+
+.fill 10000
+call func
+# CHECK-FIXUP: fixup A - offset: 0, value: func, kind: fixup_riscv_call
+# CHECK-INSTR: auipc   ra, 1048574
+# CHECK-INSTR: jalr    ra, ra, -1916
+
+.fill 20888
+call func
+# CHECK-FIXUP: fixup A - offset: 0, value: func, kind: fixup_riscv_call
+# CHECK-INSTR: auipc   ra, 1048568
+# CHECK-INSTR: jalr    ra, ra, 1764

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

test/MC/RISCV/linker-relaxation.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:
+# RELAX-FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_call
+# RELAX-FIXUP: fixup B - offset: 0, value: foo, kind: fixup_riscv_relax
 beq s1, s1, .L1
 # RELAX-RELOC: R_RISCV_BRANCH .L1 0x0
 # RELAX-FIXUP: fixup A - offset: 0, value: .L1, kind: fixup_riscv_branch