-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add full LTO build of wasi-libc and libc++ #436
Conversation
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.
As a high-level question, does Clang support embedding LLVM IR in object files? That's, for example, how the Rust compiler works without having two entirely different builds of the standard library. The single precompiled binary for the standard library supports both LTO and non-LTO builds.
cmake/wasi-sdk-sysroot.cmake
Outdated
|
||
function(define_libcxx target) | ||
define_libcxx_sub(${target} "" "" "") | ||
define_libcxx_sub(${target} "-lto" "-flto=full" "/llvm-lto/${llvm_version}") |
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.
Is this /llvm-lto/$version
convention something codified by Clang itself? Or something we're making up here in wasi-sdk?
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.
This is something that was created/added specifically for the wasm clang driver: https://github.com/llvm/llvm-project/blob/0309709a6786653da7164334c83b09c9f37b943a/clang/lib/Driver/ToolChains/WebAssembly.cpp#L206-L212
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.
Ah ok! Could that be documented here that the convention is derived from LLVM itself?
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 added a comment
cmake/wasi-sdk-sysroot.cmake
Outdated
COMMAND | ||
${MAKE} -j8 -C ${build_dir} | ||
CC=${CMAKE_C_COMPILER} | ||
AR=${CMAKE_AR} | ||
NM=${CMAKE_NM} | ||
SYSROOT=${wasi_sysroot} | ||
EXTRA_CFLAGS=${extra_cflags} | ||
TARGET_TRIPLE=${target} | ||
${extra_make_flags_lto} |
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.
Could this be split into a separate build rule with a separate build directory? I think it'd be reasonable to add an option to disable/enable the LTO builds and having it bundled in here will make it difficult to separate out. For debugging it would also be useful to have a single build target for just LTO or just without LTO.
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.
do you mean to make 4 more copies of wasi-libc source tree?
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.
Until wasi-libc supports out-of-tree builds yes I think that will be required.
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.
done
I've never heard of that before. I've only ever seen separate object files/libraries used. I'm very curious to learn about this feature! |
llvm has a concept called "fat lto object". |
The Rust compiler at least has a flag to embed bitcode in the original object file itself. It generates LLVM IR, uses that to generate an object file, then encodes the LLVM IR and puts that in the object file as well. Then when LTO is performed the compiler will recognize this type of file, extract the LLVM IR, and then process it as if it were LTO. The original object never gets passed to the linker in LTO builds. LTO works relatively differently in Rust than it does in C/C++ though. Contrary to the name it's all done within rustc itself, rustc by default doesn't rely on the linker to perform LTO. Whether or not that would work here I'm not sure, but given that there's only a single linker in the wasm ecosystem and compiler it would be pretty nice to avoid two entirely separate builds of wasi-libc/etc to be configured just right to get LTO. |
I was unaware of the "fat lto object" thing until today. Today in emscripten we generate separate versions of all the core libraries, just like is being proposed here. I think this approach used in this PR is pretty reasonable, at least for now, given that:
Perhaps one day, if we ever get around to adding "fat LTO" support we can revisit all of this. |
To be clear I wasn't saying this should all be held up to do something toally different, I was curious if something was possible. Sounds like it isn't. I think this is fine to land modulo a few small comments from myself above. |
function(define_libcxx target) | ||
execute_process( | ||
COMMAND ${CMAKE_C_COMPILER} -dumpversion | ||
OUTPUT_VARIABLE llvm_version |
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.
Can this also be called something like host_llvm_version
or clang_llvm_version
to better distinguish from clang_version
? (either that or can that variable be renamed?)
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.
my impression is clang_version should be retired. we can use -print-resource-dir instead.
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.
Can you retire it in this PR then? Otherwise this is creating a confusing state where both are available and they refer to slightly different things.
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 don't want to interfere with #445.
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.
Now that #445 is merged could this be retired?
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.
see #451
Also one thing I just remembered as well, could a test be added for this? Or also I'm not sure how the existing tests interact with this because they're already passing |
the existing tests using -flto somehow cover this. i have already fixed a bug to pass them. |
A downside: this makes four more copies of wasi-libc source tree.
the last push is a conflict resolution. |
redo #424 after cmake migration