[PATCH] D45859: [RISCV] Support "call" pseudoinstruction in the MC layer

[Alex Bradbury via Phabricator via llvm-commits]

asb requested changes to this revision.
asb added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:26-27
 
+  // Return true to preserve the symbols with relocations instead of section
+  // plus offset.
+  bool needsRelocateWithSymbol(const MCSymbol &Sym,
----------------
I'd phrase this as "Return true if the given relocation must be with a symbol rather than section plus offset".


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h:51
+  // fixup_riscv_call - A fixup representing a call attached to the auipc
+  // instruction of an adjacent jalr pair.
+  fixup_riscv_call,
----------------
It would be less ambiguous to say "auipc instruction in a pair composed of adjacent auipc+jalr instructions".


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:89
+// Expand PseudoCALL to AUIPC and JALR with relocation types.
+void RISCVMCCodeEmitter::expandFunctionCall(const MCInst &MI, raw_ostream &OS,
+                                            SmallVectorImpl<MCFixup> &Fixups,
----------------
clang-format suggests some slightly different indentation within this function.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:91
+                                            SmallVectorImpl<MCFixup> &Fixups,
+                                            const MCSubtargetInfo &STI) const {
+  MCInst TmpInst;
----------------
By expanding the 'call' at this late stage, there's no opportunity for the instruction compression MC->MC transform (which you might care about with -mno-relax and a fully resolved target). Of course there is no compressed auipc, and you'd have to be rather lucky to end up with jalr ra, ra, 0. A comment to document this limitation might be worthwhile.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp:35-36
 void RISCVMCExpr::printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const {
-  bool HasVariant = getKind() != VK_RISCV_None;
+  bool HasVariant = ((getKind() != VK_RISCV_None) &&
+                     (getKind() != VK_RISCV_CALL));
   if (HasVariant)
----------------
Is there any way we can test this has the desired effect? I had a quick play around, and it seems quite difficult to trigger this code path, but perhaps I'm missing something obvious.


================
Comment at: test/MC/RISCV/function-call.s:8
+
+call foo
+# RELOC: R_RISCV_CALL foo 0x0
----------------
This needs more tests, e.g. `call 1234` will trigger an assertion in the current patch.

I think we want test coverage for at least call 1234, calll %pcrel_lo(1234), call %pcrel_lo(foo), call %lo(1234), call %pcrel_lo(foo).


Repository:
  rL LLVM

https://reviews.llvm.org/D45859