[PATCH] D44885: [RISCV] Expand function call to auipc and jalr

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks for this as always Shiva. Mostly minor comments or questions in this review.



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:26
 
+  // Linker need relocation type with symbol to resolve offset.
+  bool needsRelocateWithSymbol(const MCSymbol &Sym,
----------------
Maybe be more explicit, e.g. "Return true if the relocation should be with a symbol rather than section".


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:29
+                               unsigned Type) const override {
+    return true;
+  }
----------------
Is this universally true, or true only for  ELF::R_RISCV_PCREL_LO12_I?


================
Comment at: lib/Target/RISCV/RISCVAsmPrinter.cpp:69
 
+void RISCVAsmPrinter::expandFunctionCall(const MachineInstr *MI) {
+  MCInst TmpInst;
----------------
It would be good to have a comment explaining that a function call will be expanded to auipc+jalr, both with relocations.


================
Comment at: lib/Target/RISCV/RISCVAsmPrinter.cpp:98
+
+  // Emit AUPIC Ra, %pcrel_hi(Func)
+  TmpInst = MCInstBuilder(RISCV::AUIPC)
----------------
AUPIC -> AUIPC


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:993
+  // TargetGlobalAddress/TargetExternalSymbol node so that legalize won't
+  // split it and then direct call can match by PseudoCALL.
   if (isa<GlobalAddressSDNode>(Callee)) {
----------------
"can be matched by"


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:994-1000
   if (isa<GlobalAddressSDNode>(Callee)) {
-    Callee = lowerGlobalAddress(Callee, DAG);
+    auto *GV = cast<GlobalAddressSDNode>(Callee)->getGlobal();
+    Callee = DAG.getTargetGlobalAddress(GV, DL, PtrVT, 0, 0);
   } else if (isa<ExternalSymbolSDNode>(Callee)) {
-    Callee = lowerExternalSymbol(Callee, DAG);
+    ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(Callee);
+    const char *Sym = S->getSymbol();
+    Callee = DAG.getTargetExternalSymbol(Sym, PtrVT, 0);
----------------
Might as well just use `dyn_cast` in the if conditions here.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:118-129
 def simm21_lsb0 : Operand<OtherVT> {
   let ParserMatchClass = SImmAsmOperand<21, "Lsb0">;
   let EncoderMethod = "getImmOpValueAsr1";
   let DecoderMethod = "decodeSImmOperandAndLsl1<21>";
 }
 
+def call_target : Operand<XLenVT> {
----------------
Maybe it's better to have `def simm21_lsb0 : Operand<XLenVT> {` and a `def br_target : Operand<OtherVT> {`.


Repository:
  rL LLVM

https://reviews.llvm.org/D44885