-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize PLT and jl_load_and_lookup calls #50745
Conversation
src/ccall.cpp
Outdated
// // putting anything complicated here: just JIT the function address | ||
// llvmf = literal_static_pointer_val(symaddr, funcptype); | ||
// } | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: delete code, do not comment it out
@@ -2363,6 +2366,8 @@ std::unique_ptr<Module> jl_create_llvm_module(StringRef name, LLVMContext &conte | |||
if (!m->getModuleFlag("Debug Info Version")) | |||
m->addModuleFlag(llvm::Module::Warning, "Debug Info Version", | |||
llvm::DEBUG_METADATA_VERSION); | |||
if (imaging_mode) | |||
m->addModuleFlag(llvm::Module::Error, "julia.imaging_mode", 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a property of the code itself (rather than a choice of the use site of the code)? That seems weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment we make different choices in imaging mode vs non-imaging mode, so I wanted to carry that over (particularly in jl_dump_native
, where we guess imaging mode from the global defaults rather than from how the code was generated).
src/jitlayers.cpp
Outdated
std::lock_guard<std::mutex> lock(*symbols_mutex); | ||
auto uit = user_symbols.find(lib); | ||
if (uit == user_symbols.end()) { | ||
void *handle = jl_get_library_(libname, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels rather risky to trigger this here, since you are holding many locks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could release the symbols_mutex lock and perhaps one iteration of the context lock, but otherwise it's technically valid I think since we never cross a lock boundary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the other problem is that jl_get_library_
is not marked as not being a safepoint, is it actually a safepoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, dlopen must be a safepoint, so this is too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, most of the ecosystem is probably deprecating code that used this pattern as soon as Elliot can finish his PR #50687
Isn't this code the base implementation of ccall/cglobal? How will #50687 deprecate this code? |
#50687 always uses the lazy function form, so this semi-eager unpredictable case doesn't occur anymore |
Does that apply to libjulia/libjulia-internal, or will that still rely on this (mostly I just don't like seeing |
That still relies on this, but not the dlopen call, so it doesn't have the arbitrary code execution issue |
@pchintalapudi This PR seems to cause a large amount of packages to fail on PkgEval. Just see the latest daily evaluation, there's many occurrences of:
e.g. https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_date/2023-08/05/JOLI.primary.log |
This removes the literal pointers from ccalls in codegen, in favor of inserting them into the JIT just before compilation. In the future, this could also permit deduplication of library and symbol globals across multiple compilation units, rather than just across a single
jl_codegen_params_t
as is done today. We also get prettier LLVM IR as a result.The deferral is implemented by adding
julia.fname
and eitherjulia.libname
orjulia.libidx
attributes to each PLT global variable and eachjl_lookup_and_load
callsite. This allows external consumers to also implement their own recognition and transformations on ccalls.Depends on #50678
cc @wsmoses since Enzyme probably cares about this.