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

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added inline comments.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:77
+      { "fixup_riscv_rvc_branch",    0,     16,  MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_riscv_call",          0,     32,  0 },
+      { "fixup_riscv_relax",         0,     32,  0 }
----------------
shiva0217 wrote:
> asb wrote:
> > 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).
> 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?
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D44886