This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Expand function call to "call" pseudoinstruction
ClosedPublic

Authored by shiva0217 on Mar 25 2018, 6:25 PM.

Details

Summary

To do this:

  1. Change GlobalAddress SDNode to TargetGlobalAddress to avoid legalizer split the symbol.
  2. Change ExternalSymbol SDNode to TargetExternalSymbol to avoid legalizer split the symbol.
  3. Let PseudoCALL match direct call with target operand TargetGlobalAddress and TargetExternalSymbol.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Mar 25 2018, 6:25 PM
apazos added inline comments.Mar 28 2018, 4:24 PM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
49 ↗(On Diff #139754)

maybe change leave relocation types -> preserve relocation types, to be in line with the term you use in other patches.

lib/Target/RISCV/RISCVAsmPrinter.cpp
73 ↗(On Diff #139754)

NULL->nullptr

106 ↗(On Diff #139754)

I think creating the instruction with RISCV::X1 would be clearer than referencing the alternative return address name.

shiva0217 updated this revision to Diff 140175.Mar 28 2018, 6:21 PM

Update patch to address Ana's comments.

zzheng added a subscriber: zzheng.Apr 3 2018, 12:04 PM
mgrang added a subscriber: mgrang.Apr 4 2018, 11:40 AM
mgrang added inline comments.
lib/Target/RISCV/RISCVAsmPrinter.cpp
106 ↗(On Diff #139754)

Although it is cleaner to create the instruction with RISCV::X1 specified explicitly, it prevents PseudoTailCall from re-using this function. PseudoTailCall is still a call but does not use X1.

@shiva0217 Could you please restore the use of "unsigned Ra" to reference the return address name?

shiva0217 updated this revision to Diff 141101.Apr 4 2018, 8:14 PM
shiva0217 edited the summary of this revision. (Show Details)

Update the patch to address @mgrang's comments.

asb added a comment.EditedApr 5 2018, 2:44 AM

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

lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
26 ↗(On Diff #141101)

Maybe be more explicit, e.g. "Return true if the relocation should be with a symbol rather than section".

29 ↗(On Diff #141101)

Is this universally true, or true only for ELF::R_RISCV_PCREL_LO12_I?

lib/Target/RISCV/RISCVAsmPrinter.cpp
69 ↗(On Diff #141101)

It would be good to have a comment explaining that a function call will be expanded to auipc+jalr, both with relocations.

98 ↗(On Diff #141101)

AUPIC -> AUIPC

lib/Target/RISCV/RISCVISelLowering.cpp
993 ↗(On Diff #141101)

"can be matched by"

994–1000 ↗(On Diff #141101)

Might as well just use dyn_cast in the if conditions here.

lib/Target/RISCV/RISCVInstrInfo.td
118–129 ↗(On Diff #141101)

Maybe it's better to have def simm21_lsb0 : Operand<XLenVT> { and a def br_target : Operand<OtherVT> {.

shiva0217 updated this revision to Diff 141593.Apr 9 2018, 1:24 AM

Update patch to address Alex's comments.

shiva0217 added inline comments.Apr 9 2018, 1:32 AM
lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
29 ↗(On Diff #141101)

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?

asb accepted this revision.Apr 12 2018, 5:55 AM

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

lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
29 ↗(On Diff #141101)

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.

lib/Target/RISCV/RISCVISelLowering.cpp
994–995 ↗(On Diff #141593)

Can use dyn_cast here too

This revision is now accepted and ready to land.Apr 12 2018, 5:55 AM
shiva0217 edited the summary of this revision. (Show Details)

Update the patch to emit pseudo call in the assembly as Alex's comments in https://reviews.llvm.org/D44886#inline-398646.

Fix typo in comments.

shiva0217 requested review of this revision.Apr 16 2018, 12:59 AM

Hi Alex. I have changed the patch to generate call pesudoinstruciton. Do you think the updated is ok?

asb requested changes to this revision.Apr 19 2018, 5:28 AM

Hi Shiva. Thanks for this, I think the best way of landing this is to split the MC and codegen changes. Could you please:

  • Make a new review that contains only the changes required to support call foo in the MC layer
  • Rebase this patch on top of that, and update the title to something like 'Expand function call to "call" pseudoinstruction'
This revision now requires changes to proceed.Apr 19 2018, 5:28 AM
shiva0217 updated this revision to Diff 143222.Apr 19 2018, 8:12 PM
shiva0217 retitled this revision from [RISCV] Expand function call to auipc and jalr to [RISCV] Expand function call to "call" pseudoinstruction.
shiva0217 edited the summary of this revision. (Show Details)

Update patch to address Alex's comments.

Why do we want to do this? As far as I can tell, it leads to worse code (e.g. br_fcmp_store_load_stack_slot materializes the address of @dummy twice).

Oh, nevermind, missed the part about linker relaxation. Please add a comment to the code which mentions that.

shiva0217 updated this revision to Diff 143498.Apr 22 2018, 7:27 PM

Update patch to address Eli's comments.

asb accepted this revision.Apr 23 2018, 9:11 AM

Thanks Shiva. I think just two outstanding issues. Some of the test files in test/CodeGen/RISCV that weren't generated with update_llc_test_checks.py have unwanted changes in this patch. With that fixed, this is looking good to me.

lib/Target/RISCV/RISCVInstrInfo.td
636–637 ↗(On Diff #143498)

I'd change the second sentence to "This is desirable, as an auipc+jalr pair with R_RISCV_CALL and R_RISCV_RELAX relocations can be be relaxed by the linker if the offset fits in a signed 21-bit immediate."

This revision is now accepted and ready to land.Apr 23 2018, 9:11 AM
shiva0217 updated this revision to Diff 143691.Apr 24 2018, 1:57 AM

Update patch to address Alex's comments.

@shiva0217 Could you please rebase and commit this patch? My tail call patch D45395 depends on this patch.

@shiva0217 Could you please rebase and commit this patch? My tail call patch D45395 depends on this patch?

Hi @mgrang, I'll update the patches in the next few hours.

shiva0217 updated this revision to Diff 143857.Apr 24 2018, 7:57 PM

Rebase the patch due the updated on https://reviews.llvm.org/D45859.

This revision was automatically updated to reflect the committed changes.