This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Lower extern_weak symbols using the GOT for the medany model
ClosedPublic

Authored by jrtc27 on Aug 2 2021, 7:44 AM.

Details

Summary

Such symbols may be undefined at link time and thus resolve to 0, which
may be further than 2GiB away from PC, causing the immediate to be out
of range for PC-relative addressing. Using the GOT avoids this, and is
the approach taken by AArch64.

Diff Detail

Event Timeline

jrtc27 created this revision.Aug 2 2021, 7:44 AM
jrtc27 requested review of this revision.Aug 2 2021, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2021, 7:44 AM
MaskRay accepted this revision.Aug 2 2021, 9:36 AM

Thanks! Two nits

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5207

PseudoLGA

5233

IsExternWeak can be omitted

This revision is now accepted and ready to land.Aug 2 2021, 9:36 AM
jrtc27 updated this revision to Diff 363499.Aug 2 2021, 9:36 AM

Fixed copy paste error in comment

jrtc27 marked an inline comment as done.Aug 2 2021, 9:38 AM
jrtc27 added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5207

Already spotted and fixed :)

5233

What do you mean? If it's omitted then it defaults to false and we get the old codegen

MaskRay added inline comments.Aug 2 2021, 9:39 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5233

Change the argument. The variable can be removed.

jrtc27 marked an inline comment as done.Aug 2 2021, 9:42 AM
jrtc27 added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5233

Well you could say the same about Ty and IsLocal, they're both used exactly once so could be inlined, but IMO this keeps it more readable; nested function calls can get rather messy.

MaskRay added inline comments.Aug 2 2021, 9:45 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5233

IsExternWeak is a good candidate. Its expr isn't very long. IsLocal is long and leaving it as is is fine.

reames added a subscriber: reames.Jul 25 2022, 12:00 PM

It looks like this has been approved for roughly a year and yet hasn't landed. Could we get this rebased and landed?

I'm aware this is somewhat still an area of controversy in the psABI, but the GOT based lowering here is conservatively correct regardless of outcome there. If we do settle on the GOT required answer - which looks likely, having this in as early as possible should help minimize migration pain. If we decide on requiring the relaxation, this change does no harm and could be reverted later if desired. The only reason I can see to hold this if we think there's a potential performance concern, but if so, we really need to know that for the psABI discussion and exposing it by landing this and seeing what falls out seems like still a net win.

There is no controversy. The behavior on most targets for external weak symbol is to use GOT.

I was holding off until the LGA pseudo was accepted in riscv-asm-manual, which seems to finally have happened in March

jrtc27 updated this revision to Diff 447492.Jul 25 2022, 3:12 PM

Rebased, including to use riscv_lga ISD node instead and create a memop like for la

arichardson accepted this revision.Jul 27 2022, 10:45 AM

LGTM if everyone else is happy with it.

Would it make sense to backport this patch series to the 15 release? I'd quite like to be able to make use of this.

LLVM 16 has been released, can we get this into 17?

asb accepted this revision.Apr 12 2023, 1:47 AM

I think all questions and review comments were resolved - anything you can see blocking this now @jrtc27?

I think all questions and review comments were resolved - anything you can see blocking this now @jrtc27?

I think it just didn’t get landed because it needs rebasing to adapt to more recent changes in L(L)A handling. Will try and update in the coming days.

jrtc27 updated this revision to Diff 522424.May 15 2023, 9:09 PM

Rebase on updated test

MaskRay accepted this revision.May 15 2023, 10:25 PM

Looks great!

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5207

Extern weak symbols, if unresolved, must be addressed indirectly ...

extern_weak is the IR representation related to the current module. Another TU may provide a definition. The original sentence is imprecise.

MaskRay added inline comments.May 15 2023, 10:32 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5207

Hmm, better wording may be that "An unresolved undefined weak symbol has a value of zero, which may not be within +-2GiB of PC. Use a GOT-generating code sequence so that the zero address is representable." This may feel verbose. Feel free to simplify.

evandro removed a subscriber: evandro.May 17 2023, 3:56 PM
This revision was landed with ongoing or failed builds.May 31 2023, 10:49 AM
This revision was automatically updated to reflect the committed changes.