Page MenuHomePhabricator

[RISCV] Support NonLazyBind
Needs ReviewPublic

Authored by M_ximus on Aug 16 2019, 4:31 AM.

Details

Reviewers
asb
jrtc27
lenary
Summary

This patch is needed to load call address from shared library without the PLT(load from the GOT).

Diff Detail

Event Timeline

M_ximus created this revision.Aug 16 2019, 4:31 AM

@jrtc27 your input here would be valuable.

jrtc27 requested changes to this revision.Sep 3 2019, 3:03 AM

Looks sensible in general, just various minor points.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2187–2189

This comment probably needs updating to reflect the fact that GlobalAddress/ExternalSymbol may now become a normal node and indirectly called if using NonLazyBind.

2194

This should only be an additional 4 spaces of indentation.

2195–2199

Please remove this extra line.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
58

The dyn_cast seems a bit weird but AArch64 and X86 (the only other backends supporting this) both do that so I guess it's fine. However, the MO_CALL fallback case is a change of behaviour; non-function non-DSO-locals would previously have still been called with an MO_PLT, so the F && should be part of the F->hasFnAttribute condition.

llvm/lib/Target/RISCV/RISCVSubtarget.h
71

"the PLT" and "the GOT".

llvm/test/CodeGen/RISCV/no-plt.ll
2

Please do this for -mtriple=riscv32 and -mtriple=riscv64 (no -linux-gnu is generally used in the CodeGen tests unless it's necessary), and use a single - for -relocation-model.

7

Please use update_llc_test_checks.py (and my own personal opinion is this would look nicer with a lowercase plt in the function name).

This revision now requires changes to proceed.Sep 3 2019, 3:03 AM
jrtc27 added inline comments.Sep 3 2019, 3:09 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2197

The 0 should be false since IsLocal is a bool, and ideally /*IsLocal=*/false to document what it means.

M_ximus updated this revision to Diff 220666.Sep 18 2019, 7:47 AM
M_ximus edited the summary of this revision. (Show Details)

All mistakes in the patch were fixed.

jrtc27 requested changes to this revision.Sep 18 2019, 9:43 AM

A couple of minor remaining points.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2187–2189

Actually I should have been more accurate; only GlobalAddress nodes will become indirect calls, ExternalSymbols will always be direct calls through TargetExternalSymbol nodes, since this only changes the GlobalAddress branch (as you need the function to get its attributes).

llvm/test/CodeGen/RISCV/no-plt.ll
2

Still missing a -mtriple=riscv32 line.

This revision now requires changes to proceed.Sep 18 2019, 9:43 AM
M_ximus updated this revision to Diff 220870.Sep 19 2019, 8:13 AM
M_ximus marked 8 inline comments as done.

I added -mtriple=riscv32 line and fixed the comment

lenary resigned from this revision.Jan 14 2021, 10:26 AM

-fno-plt essentially inlines the PLT entry into the call site. For x86-64, there are benefits if many call sites are defined in different DSOs.

If the callee is actually defined, -fno-plt is actually pessimization. I don't think -fno-plt is useful. Usually people just need some __attribute__((noplt)) on libc functions. Clang has not implemented it, though.

rkruppe removed a subscriber: rkruppe.Jan 15 2021, 12:55 AM