[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optimize PLT and jl_load_and_lookup calls #50745

Merged
merged 4 commits into from
Aug 3, 2023
Merged

Optimize PLT and jl_load_and_lookup calls #50745

merged 4 commits into from
Aug 3, 2023

Conversation

pchintalapudi
Copy link
Member

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 either julia.libname or julia.libidx attributes to each PLT global variable and each jl_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.

@pchintalapudi pchintalapudi added compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM labels Aug 1, 2023
vtjnash
vtjnash reviewed Aug 1, 2023
View reviewed changes
src/ccall.cpp Outdated
// // putting anything complicated here: just JIT the function address
// llvmf = literal_static_pointer_val(symaddr, funcptype);
// }
// }
Copy link
Sponsor Member

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

vtjnash
vtjnash approved these changes Aug 1, 2023
View reviewed changes
src/codegen.cpp
@@ -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);
Copy link
Sponsor Member

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

Copy link
Member Author

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

vtjnash
vtjnash reviewed Aug 1, 2023
View reviewed changes
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);
Copy link
Sponsor Member

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

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Sponsor Member

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

vtjnash
vtjnash reviewed Aug 1, 2023
View reviewed changes
Copy link
Sponsor Member
@vtjnash vtjnash left a 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

@pchintalapudi
Copy link
Member Author

Isn't this code the base implementation of ccall/cglobal? How will #50687 deprecate this code?

@vtjnash
Copy link
Sponsor Member
vtjnash commented Aug 1, 2023 •
edited
Loading

#50687 always uses the lazy function form, so this semi-eager unpredictable case doesn't occur anymore

@pchintalapudi
Copy link
Member Author

Does that apply to libjulia/libjulia-internal, or will that still rely on this (mostly I just don't like seeing call inttoptr 140234234... when really I'm looking at an array allocation).

@vtjnash
Copy link
Sponsor Member
vtjnash commented Aug 1, 2023

That still relies on this, but not the dlopen call, so it doesn't have the arbitrary code execution issue

@pchintalapudi pchintalapudi merged commit c03a346 into master Aug 3, 2023
4 of 6 checks passed
@pchintalapudi pchintalapudi deleted the pc/jit-later branch August 3, 2023 16:40
@maleadt
Copy link
Member
maleadt commented Aug 12, 2023

@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:

julia: /source/usr/include/llvm/Support/Casting.h:578: decltype(auto) llvm::cast(From*) [with To = llvm::ConstantExpr; From = llvm::Value]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

[506] signal (6.-6): Aborted
in expression starting at /home/pkgeval/.julia/packages/JOLI/S3LJy/src/JOLI.jl:57
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7f0dc843f40e)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
cast<llvm::ConstantExpr, llvm::Value> at /source/usr/include/llvm/Support/Casting.h:578 [inlined]
operator() at /source/src/jitlayers.cpp:1597
optimizeDLSyms at /source/src/jitlayers.cpp:2032 [inlined]
_jl_compile_codeinst at /source/src/jitlayers.cpp:230
jl_generate_fptr_impl at /source/src/jitlayers.cpp:528
jl_compile_method_internal at /source/src/gf.c:2475 [inlined]
jl_compile_method_internal at /source/src/gf.c:2363
gen_cfun_wrapper at /source/src/codegen.cpp:6090
emit_cfunction at /source/src/codegen.cpp:6727 [inlined]
emit_expr at /source/src/codegen.cpp:5564
emit_ssaval_assign at /source/src/codegen.cpp:5135
emit_stmtpos at /source/src/codegen.cpp:5370 [inlined]
emit_function at /source/src/codegen.cpp:8462

e.g. https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_date/2023-08/05/JOLI.primary.log

@pchintalapudi pchintalapudi mentioned this pull request Aug 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash approved these changes

@maleadt maleadt Awaiting requested review from maleadt

Assignees
No one assigned
Labels
compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pchintalapudi @vtjnash @maleadt