lto: use the object crate to load bitcode sections
#115136
Conversation
Upstream change llvm/llvm-project@6b539f5 changed `isSectionBitcode` works and it now only respects `.llvm.lto` sections instead of also `.llvmbc`, which it says was never intended to be used for LTO. We already have the object crate handy, so it's easy to use that to load the section. We have to sniff for raw bitcode files by hand, as `object` doesn't support those, but that's easy enough. I also look for both .llvmbc and .llvm.lto sections so that the behavior of the new code matches the old LLVM behavior exactly, though .llvm.lto sections seem to not (currently) be covered by tests. r? @nikic @rustbot label: +llvm-main
rustbot
added
S-waiting-on-review
|
@nikic do you need anything else from me? I see you didn't tell bors to merge this. |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
⌛ Testing commit ee81e0d with merge 3fc4e39defc6373c71bdab390a7e0ac2ee38b4cc... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
Otherwise we lose the ability to do crate-level LTO for wasm targets now that we're loading our own bitcode sections instead of using LLVM to do it.
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
@nikic back to you, I'm afraid: we had to enable the Sigh. |
This comment has been minimized.
This comment has been minimized.
We already allow enough cousins of this license I can't imagine it being a problem.
|
@bjorn3 should I just go back to calling the LLVM method if the file magic lines up with wasm? |
|
I think the bitcode will likely stay in a custom section, so using object will work fine. And if it no longer ends up in a custom section in the future you could either switch back or hopefully object supports actually parsing wasm object with the linking custom section files by then. |
|
Okay, so it sounds like this is probably fine for now? |
|
Do I understand correctly that this is just waiting for review from @nikic now? |
|
Yes, as far as I know I'm waiting on a review.
To my knowledge, there's no way to gate the wasm dep, yes.
…On Mon, Sep 4, 2023, 10:34 klensy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In compiler/rustc_codegen_llvm/Cargo.toml
<#115136 (comment)>:
> @@ -14,6 +14,7 @@ measureme = "10.0.0"
object = { version = "0.32.0", default-features = false, features = [
"std",
"read",
+ "wasm",
Today it seems not possible
<rust-lang/cargo#1197> to gate this behind wasm
target, to not to pull this on other platforms?
—
Reply to this email directly, view it on GitHub
<#115136 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAE6LMVQGJBQSHN4JCNDBDXYXRFRANCNFSM6AAAAAA33UDO5M>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
r=me on the code change, but I believe the license needs to go under EXCEPTIONS, because it doesn't comply with Rust's license policy, so it's only allowed by dint of being a rustc only dep. Would be great if someone (@wesleywiser?) can confirm the license situation here. |
|
@wesleywiser do you need anything from me on the license front here? |
|
We just tried cherry-picking this in Chromium, but it seems that on Mac this actually breaks tests/codegen/lto-removes-invokes.rs rather than fixing it (https://crbug.com/1476119#c3). Is there some CI that runs on Mac that can be triggered here? |
CI on macOS will be run during the merge testing so if there's a failure there then it'll be fixed before this gets merged. |
I think this is okay because the dependency is Apache 2.0 with LLVM Exception licensed, which is compatible with rustc's license. |
|
@bors r=nikic,davidtwco |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 3259458 with merge c13be90883040fb63be9188e57aab4df0b1689d3... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
So, it looks like wasmparser needs AtomicU64 in the current implementation at least. Given that using the object crate seems to be more complicated than I anticipated, I'd be fine with staying with an LLVM-based implementation (just with our own implementation of findBitcodeInMemBuffer looking for different sections). |
|
@rustbot label: -llvm-main |
rustbot
removed
the
llvm-main
|
Obsoleted by #115641 |
lto: load bitcode sections by name Upstream change llvm/llvm-project@6b539f5 changed `isSectionBitcode` works and it now only respects `.llvm.lto` sections instead of also `.llvmbc`, which it says was never intended to be used for LTO. We instead load sections by name, and sniff for raw bitcode by hand. This is an alternative approach to rust-lang#115136, where we tried the same thing using the `object` crate, but it got too fraught to continue. r? `@nikic` `@rustbot` label: +llvm-main
Upstream change
llvm/llvm-project@6b539f5 changed
isSectionBitcodeworks and it now only respects.llvm.ltosections instead of also.llvmbc, which it says was never intended to be used for LTO. We already have the object crate handy, so it's easy to use that to load the section.We have to sniff for raw bitcode files by hand, as
objectdoesn't support those, but that's easy enough. I also look for both .llvmbc and .llvm.lto sections so that the behavior of the new code matches the old LLVM behavior exactly, though .llvm.lto sections seem to not (currently) be covered by tests.Fixes #115069
r? @nikic
@rustbot label: +llvm-main