-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Upgrade Emscripten targets to use upstream LLVM backend #63649
Conversation
TODO: update CI to install and use upstream emscripten to run tests. The ui/statics/static-mut-foreign.rs test will not pass without rust-lang/libc#1478 and possibly a patch to wasm-ld. There are also some outstanding undiagnosed test failures on js-unknown-emscripten, so wasm2js will probably require more bugfixes before this can land as well. |
// wasm32 can't build the test helpers | ||
if !target.contains("wasm32") { | ||
builder.ensure(native::TestHelpers { target }); | ||
} |
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'm not sure which LLVM versions rust supports, so this may not be correct. This change was necessary to uncover all the ABI problems, though.
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 this may fail on CI for the wasm32-unknown-unknown target, but I think that it'll probably work in that it'll generate a native library with ELF code (using clang
) and then we'd just ignore it during the test suite. In that sense let's just ignore this for now and if it causes problems on CI we can add it back, but only for the non-emscripten target
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 believe this is the line that's failing on CI, this'll need to be disabled still for wasm32-unknown-unknown I believe
Thanks for the PR @tlively! I think we may want to hold off on the target renaming yet, I suspect there's probably enough users of the target that this is already going to be somewhat breaking but changing names may be a bit aggressive for a first step. I definitely agree that the new naming is better, just thinking we may want to roll it out a bit slowly (also since it's just purely naming). The main thing I'm worried about is if anyone's using For CI, You can edit Would this be able to remove |
ping from triage @tlively, any updates on this? |
Still on my radar! I've been mostly working on getting the CI to pass on the related PR rust-lang/libc#1478. I'm pretty sure that needs to be merged before this one, but I'm not sure how libc gets rolled into rustc. I will be updating this PR in response to the review comments next week. |
|
||
fn classify_ret_ty<Ty>(ret: &mut ArgType<'_, Ty>) { | ||
ret.extend_integer_width_to(32); |
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 think this should be kept.
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 explain why? There’s no need to expand i8s and i16s to i32s during the lowering to LLVM IR because LLVM will perform the necessary type legalization itself. In fact, it’s better not to do the expansion here so that legalization is handled by the same system for both C/C++ and Rust.
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 agree with @bjorn3, Clang appears to do this: https://github.com/llvm/llvm-project/blob/d01b4a786271c9ce9df43add56ec2e3fb40ce920/clang/lib/CodeGen/TargetInfo.cpp#L716
} | ||
|
||
fn classify_arg_ty<Ty>(arg: &mut ArgType<'_, Ty>) { | ||
arg.extend_integer_width_to(32); |
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.
Same here.
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 agree with @bjorn3, Clang appears to do this: https://github.com/llvm/llvm-project/blob/d01b4a786271c9ce9df43add56ec2e3fb40ce920/clang/lib/CodeGen/TargetInfo.cpp#L701
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.
Yeah, it's confusing that this happens in both the frontend and the backend. I suppose if we're trying to match clang here then it would make sense to do the expansion, although practically it shouldn't make a difference.
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 think the difference is we pick sign- or zero- extension depending on the type, which LLVM doesn't know, so presumably it would be correct for unsigned integers but not signed, or vice-versa.
I didn't really like this entire dance but IIRC @rkruppe explained at some point that it makes life easier for architectures with e.g. 32-bit registers that you want to do 16-bit or 8-bit operations within, if everything is appropriately extended with its individual signedness.
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.
Yeah that makes sense actually 👍
@@ -377,6 +377,7 @@ | |||
# but you can also optionally enable the "emscripten" backend for asm.js or | |||
# make this an empty array (but that probably won't get too far in the | |||
# bootstrap) | |||
# FIXME: remove the obsolete emscripten backend option. |
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.
Please keep the codegen-backends
option when you remove that.
☔ The latest upstream changes (presumably #64246) made this pull request unmergeable. Please resolve the merge conflicts. |
Update: I've undone the asmjs => js renaming locally and everything is working well. I'm now experimenting with enabling Emscripten's exception handling to allow panic=unwind, but am running into some bug, probably in LLVM. If that takes much longer I will update this PR to use panic=abort and leave panic=unwind support for the future. |
Ah, of course the exception handling doesn't work because https://reviews.llvm.org/rG5c3cdef84b82464756bb571c13c31cf7773860c3 has not been merged into Rust's LLVM fork. I'll create a separate issue for that and update this PR to use panic=abort since I don't know how long that will take. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Oops, looks like I accidentally changed all the submodules 🙃 |
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.
Looks great to me!
Would you be ok editing src/ci/azure-pipelines/pr.yml
to include the asmjs/wasm entries of src/ci/azure-pipelines/auto.yml
temporarily? That would ensure that these builds all still pass on this PR, and then we can back that out just before merging with bors.
# asmjs-specific backend assertion trip), so disable assertions for these | ||
# tests. | ||
ENV NO_LLVM_ASSERTIONS=1 | ||
ENV NO_DEBUG_ASSERTIONS=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.
FWIW I think we trimmed down the tests run on the emscripten builder because of how long it took, so this may want to preserve the lack of assertions and running just a few test suites
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 hope is that with the upstream LLVM backend instead of Fastcomp the performance should be similar to that of other targets so we can give it the full testing treatment, but I'd be happy to put these limits back if that does not pan out.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I haven't done much analysis of the timing, but those did run for a bit of a long time. It looks like there's some asmjs failures, but perhaps we could switch back to running a subset of tests and then expand it later if necessary? We're already bleeding for time in a lot of areas so I don't think we can afford taking too much longer. Otherwise I'm realizing now that |
The The The other three test failures were in new tests that had |
New |
If tests require something like a newer |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
We believe the link failure is due to a recent emscripten regression, so this will have to wait for a bit before the libstd, libcore, or liballoc tests can be run. |
Is the only error here you're referencing the part about |
Interestingly rpath passing is already turned off for the emscripten target, and yet an rpath argument is still being passed. The regression with emscripten is that we used to ignore rpath arguments but now we erroneously pass them through. So we can either wait for that to be fixed or figure out why an rpath is being passed in the first place. I will dive in deeper once I finish some of the other stuff I'm doing. |
Updates LLVM to pick up the cherry-picked support for correctly handling exception handling with aggregates passed by value. This will be necessary to continue to support Emscripten's exception handling once we switch using Emscripten's LLVM backend. See rust-lang#63649.
…exceptions, r=nikic Update LLVM for Emscripten exception handling support Updates LLVM to pick up the cherry-picked support for correctly handling exception handling with aggregates passed by value. This will be necessary to continue to support Emscripten's exception handling once we switch using Emscripten's LLVM backend. See rust-lang#63649.
…exceptions, r=nikic Update LLVM for Emscripten exception handling support Updates LLVM to pick up the cherry-picked support for correctly handling exception handling with aggregates passed by value. This will be necessary to continue to support Emscripten's exception handling once we switch using Emscripten's LLVM backend. See rust-lang#63649.
- Compatible with Emscripten 1.38.46-upstream or later upstream. - Refactors the Emscripten target spec to share code with other wasm targets. - Replaces the old incorrect wasm32 C call ABI with the correct one, preserving the old one as wasm32_bindgen_compat for wasm-bindgen compatibility. - Updates the varargs ABI used by Emscripten and deletes the old one. - Removes the obsolete wasm32-experimental-emscripten target. - Uses EMCC_CFLAGS on CI to avoid the timeout problems with rust-lang#63649.
Upgrade Emscripten targets to use upstream LLVM backend - Compatible with Emscripten 1.38.46-upstream or later upstream. - Refactors the Emscripten target spec to share code with other wasm targets. - Replaces the old incorrect wasm32 C call ABI with the correct one, preserving the old one as wasm32_bindgen_compat for wasm-bindgen compatibility. - Updates the varargs ABI used by Emscripten and deletes the old one. - Removes the obsolete wasm32-experimental-emscripten target. - Uses EMCC_CFLAGS on CI to avoid the timeout problems with #63649. r? @alexcrichton
wasm32-experimental-emscripten was removed in rust-lang/rust#63649 (or rust-lang/rust#65251).
This is no longer used since rust-lang#63649
…r=spastorino Remove leftover from emscripten fastcomp support This is no longer used since rust-lang#63649
…r=spastorino Remove leftover from emscripten fastcomp support This is no longer used since rust-lang#63649
targets.
version, which is correct for both wasm32 and JS.
because supporting unwinding will require an LLVM patch.