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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

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



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


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:78
+      { "fixup_riscv_call",          0,     32,  0 },
+      { "fixup_riscv_relax",         0,     32,  0 }
     };
----------------
Shouldn't bits = 0 for relax, given that it's purely a tag?


================
Comment at: test/MC/RISCV/linker-relaxation.s:4
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=-relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELOC %s
+# RUN: llvm-mc -triple riscv32 -mattr=+relax < %s -show-encoding \
----------------
Might be clearer as NORELAX-RELOC


================
Comment at: test/MC/RISCV/linker-relaxation.s:7
+# RUN:     | FileCheck -check-prefix=RELAX-FIXUP %s
+
+.long foo
----------------
Could you please add some riscv64 RUN lines


================
Comment at: test/MC/RISCV/linker-relaxation.s:12
+call foo
+# RELOC: R_RISCV_CALL foo 0x0
+# RELAX-RELOC: R_RISCV_CALL foo 0x0
----------------
We should check NORELAX-RELOC-NOT: R_RISCV_RELAX


Repository:
  rL LLVM

https://reviews.llvm.org/D44886