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

[Alex Bradbury via Phabricator via llvm-commits]

asb accepted this revision.
asb added a comment.
This revision is now accepted and ready to land.

Thanks Shiva. This looks good to me - just two additional minor suggestions prior to commit (add a TODO comment and switch to using dyn_cast in one place).



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:29
+                               unsigned Type) const override {
+    return true;
+  }
----------------
shiva0217 wrote:
> asb wrote:
> > Is this universally true, or true only for  ELF::R_RISCV_PCREL_LO12_I?
> It seems that gcc always emit relocations with symbols which could observe by readelf object files. So it's probably universally true. I changed the comment as "Return true to preserve symbol with relocations instead of section plus offset". Is it ok?  
That seems very conservative. It's probably the right starting point, but maybe a TODO comment would be worthwhile. .g. 'TODO: this is very conservative, update once RISC-V psABI requirements are clarified'. Although the current binutils linker seems to have many limitations, wouldn't we expect that needsRelocateWithSymbol can be false in general as long as relaxation is disabled?

Returning true unconditionally seems sensible for now, but lets leave a note to come back to this.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:994-995
+  // split it and then direct call can be matched by PseudoCALL.
   if (isa<GlobalAddressSDNode>(Callee)) {
-    Callee = lowerGlobalAddress(Callee, DAG);
-  } else if (isa<ExternalSymbolSDNode>(Callee)) {
-    Callee = lowerExternalSymbol(Callee, DAG);
+    auto *GV = cast<GlobalAddressSDNode>(Callee)->getGlobal();
+    Callee = DAG.getTargetGlobalAddress(GV, DL, PtrVT, 0, 0);
----------------
Can use dyn_cast here too


Repository:
  rL LLVM

https://reviews.llvm.org/D44885