Skip to content
This repository has been archived by the owner. It is now read-only.

/llvm forked from luqmana/llvm

Squash of fastcomp commit 4105790f1549808c1f1daa5250b4ada5f41a5c02 #50

Merged
merged 2 commits into from Sep 7, 2016

Conversation

Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@brson @rust-highfive @alexcrichton @kripken
@brson
Copy link

brson commented Sep 6, 2016 •
edited

This is the entire content of the emscripten fastcomp LLVM fork, which it uses to generate asm.js and which can also be used to generate wasm. By importing this into our tree we can do the same, by emitting LLVM IR that emscripten's emcc compiler understands.

The vast majority of the code here belongs to the pnacl IR legalizer and to the asmjs backend, though there are a few bits that touch common code. As such, I expect this to be relatively easy to maintain, the maintenance model being:

  • When either Rust or emscripten need to upgrade LLVM, we do it at the same time. This can involve some experimentation to find a commit that works for both of us.
  • We squash their entire commit history after their merge base into one, giving it a name containing the sha of their head commit, then cherry pick it onto our patch set.
  • If ever the upgrade becomes too difficult we can just exclude the fastcomp patch and temporarily drop asmjs/wasm support.

The only reason I expect emscripten to need to upgrade LLVM in the future is to move to the upstream wasm backend, which we will also want. So there is in fact the possibility we will be looking forward to quite a bit of llvm churn in the near future.

Once the LLVM wasm backend is complete we can consider dropping this patch, though at that time will either need to also drop asm.js support or invest considerable effort into a wasm2asmjs code path.

Once this is merged and the libc patches for wasm merged, I will submit a patch to Rust adding more-or-less working asmjs/wasm32 targets.

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton Sep 6, 2016

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link

rust-highfive commented Sep 6, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.
@rust-highfive

This comment has been minimized.

Sign in to view
Copy link

rust-highfive commented Sep 6, 2016

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against rust-llvm-2016-07-09. Please double check that you specified the right target!
lib/Transforms/InstCombine/InstCombineVectorOps.cpp Outdated
// is quite desirable.
if (isSplat || newMask == LHSMask || newMask == RHSMask || newMask == Mask ||
true)
{

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Sep 6, 2016

Member

This seems pretty suspicious in the sense that it's a change made with the assumption that LLVM is only generating asmjs. I wonder if we should back this out?

This comment has been minimized.

Sign in to view
Copy link
@brson

brson Sep 7, 2016

Author

I've checked with @sunfish and we can revert this with little performance impact on asmjs. I will do so.
lib/Transforms/InstCombine/InstCombineCompares.cpp
// @LOCALMOD-BEGIN
// We don't want to introduce non-power-of-two integer sizes for PNaCl's
// stable wire format, so modify this transformation for NaCl.
isPowerOf2_32(TypeBits - Amt) && (TypeBits - Amt) >= 8 &&

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Sep 6, 2016

Member

Although this seems suspicious, it also seems like something that we wouldn't hit anyway (we never use e.g. i55) so this appears fine

This comment has been minimized.

Sign in to view
Copy link
@brson

brson Sep 7, 2016

Author

I agree this is probably fine. Would be nice to have a comment from @sunfish or @kripken explaining what this does for asmjs.

This comment has been minimized.

Sign in to view
Copy link
@kripken

kripken Sep 7, 2016

It looks like code from our pnacl legacy, avoiding weird nonstandard integers (which we don't know how to legalize). I'm not sure of more details, but to be safe, you could condition it on the target being emscripten.
lib/Transforms/IPO/GlobalOpt.cpp Outdated
if (Constant *SOVConstant = dyn_cast<Constant>(GS.StoredOnceValue)) {
if (GS.Ordering == AtomicOrdering::NotAtomic) {
if (TryToShrinkGlobalToBoolean(GV, SOVConstant)) {
if (TryToAddRangeMetadata(GV, SOVConstant)) { // XXX EMSCRIPTEN

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Sep 6, 2016

Member

I... don't know what's going on here, but... I guess can't be too bad?

This comment has been minimized.

Sign in to view
Copy link
@brson

brson Sep 7, 2016

Author

Supposedly this only affects lto.

This comment has been minimized.

Sign in to view
Copy link
@kripken

kripken Sep 7, 2016

I think this annoyed us because it turns globals into booleans which ended up adding overhead for us (masking, etc.) on some benchmark. I doubt it makes much different natively. But as in the others, might make sense to condition it on the target being emscripten I guess.
lib/IR/Verifier.cpp Outdated
@@ -606,10 +606,10 @@ void Verifier::visitGlobalVariable(const GlobalVariable &GV) {
GV.hasAvailableExternallyLinkage(),
"Global is marked as dllimport, but not external", &GV);

if (!GV.hasInitializer()) {
//if (!GV.hasInitializer()) { // XXX EMSCRIPTEN - do not do extra verification below, 40x slower linking on some big projects

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Sep 6, 2016

Member

Seems like we may still want this

This comment has been minimized.

Sign in to view
Copy link
@brson

brson Sep 7, 2016

Author

I've reverted this.
include/llvm/InitializePasses.h
@@ -336,6 +336,69 @@ void initializeVirtRegMapPass(PassRegistry&);
void initializeVirtRegRewriterPass(PassRegistry&);
void initializeWholeProgramDevirtPass(PassRegistry &);
void initializeWinEHPreparePass(PassRegistry&);

// @LOCALMOD-BEGIN
void initializeAddPNaClExternalDeclsPass(PassRegistry&);

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Sep 6, 2016

Member

So just to confirm, we currently believe that these passes will only run for the emscripten target? That is, if we generate x86_64 linux code, these'll all be turned off by default?

This comment has been minimized.

Sign in to view
Copy link
@kripken

kripken Sep 7, 2016

Yes, the only place those initialize* methods are called is in the asmjs backend. They can't affect normal builds.

@brson brson referenced this pull request Sep 7, 2016

Closed

Initial webassembly support via LLVM #36317

9 of 17 tasks complete

@brson brson force-pushed the brson:fastcomp-squash branch from f19e5a4 to af0684e Sep 7, 2016

@brson

This comment has been minimized.

Sign in to view
Copy link
Author

brson commented Sep 7, 2016

@alexcrichton I've added a patch to revert two of the changes and determined that the others should not affect us.

@brson brson force-pushed the brson:fastcomp-squash branch from af0684e to 8d07d6d Sep 7, 2016

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Sep 7, 2016

Tally ho!

@alexcrichton alexcrichton merged commit c3bfb2c into rust-lang:rust-llvm-2016-07-09 Sep 7, 2016

@brson brson referenced this pull request Sep 8, 2016

Merged

Working asmjs and wasm targets #36339

@alexcrichton alexcrichton referenced this pull request Sep 8, 2016

Closed

Does our LLVM need the fastcomp backend for emscripten? #36356

bors added a commit to rust-lang/rust that referenced this pull request Sep 28, 2016

@bors
Auto merge of #36339 - brson:emscripten-new, r=alexcrichton 
Working asmjs and wasm targets

This patch set results in a working standard library for the asmjs-unknown-emscripten and wasm32-unknown-emscripten targets. It is based on the work of @badboy and @rschulman.

It does a few things:

- Updates LLVM with the emscripten [fastcomp](rust-lang/llvm#50) patches, which include the pnacl IR legalizer and the asm.js backend. This patch is thought not to have any significant effect on existing targets.
- Teaches rustbuild to correctly link C code with emscripten
- Updates gcc-rs to work correctly with emscripten
- Teaches rustbuild to run crate tests for emscripten with node
- Modifies Thread::new to return an error on emscripten, to facilitate debugging a common failure mode
- Modifies libtest to run in single-threaded mode for emscripten
- Ignores a host of tests that don't work yet, mostly dealing with threads and I/O
- Updates libc with wasm32 definitions (presently the same as asmjs)
- Adds a wasm32-unknown-emscripten target that feeds the output of LLVM's asmjs backend through emcc to generate wasm

Notes and caveats:

- This is only known to work with `--enable-rustbuild`.
- The wasm32 target can't be tested correctly yet because of issues in compiletest and limitations in node emscripten-core/emscripten#4542, but hello.rs does seem to work when run on node via the binaryen interpreter
- This requires an up to date installation of the emscripten sdk from its incoming branch
- Unwinding is very broken
- When enabling the emscripten targets jemalloc is disabled for all targets, which results in test failures for the host

Next steps are to fix the jemalloc issue, start building the two emscripten targets on the auto builders, then start producing nightlies.

#36317 tracks work on this.

Fixes #36515
Fixes #36515
ebd61d6

bors added a commit to rust-lang/rust that referenced this pull request Oct 1, 2016

@bors
Auto merge of #36339 - brson:emscripten-new, r=alexcrichton 
Working asmjs and wasm targets

This patch set results in a working standard library for the asmjs-unknown-emscripten and wasm32-unknown-emscripten targets. It is based on the work of @badboy and @rschulman.

It does a few things:

- Updates LLVM with the emscripten [fastcomp](rust-lang/llvm#50) patches, which include the pnacl IR legalizer and the asm.js backend. This patch is thought not to have any significant effect on existing targets.
- Teaches rustbuild to correctly link C code with emscripten
- Updates gcc-rs to work correctly with emscripten
- Teaches rustbuild to run crate tests for emscripten with node
- Modifies Thread::new to return an error on emscripten, to facilitate debugging a common failure mode
- Modifies libtest to run in single-threaded mode for emscripten
- Ignores a host of tests that don't work yet, mostly dealing with threads and I/O
- Updates libc with wasm32 definitions (presently the same as asmjs)
- Adds a wasm32-unknown-emscripten target that feeds the output of LLVM's asmjs backend through emcc to generate wasm

Notes and caveats:

- This is only known to work with `--enable-rustbuild`.
- The wasm32 target can't be tested correctly yet because of issues in compiletest and limitations in node emscripten-core/emscripten#4542, but hello.rs does seem to work when run on node via the binaryen interpreter
- This requires an up to date installation of the emscripten sdk from its incoming branch
- Unwinding is very broken
- When enabling the emscripten targets jemalloc is disabled for all targets, which results in test failures for the host

Next steps are to fix the jemalloc issue, start building the two emscripten targets on the auto builders, then start producing nightlies.

#36317 tracks work on this.

Fixes #36515
Fixes #36515
Fixes #36356
8b00355
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.