Page MenuHomePhabricator

[WebAssembly] promote from experimental to normal target
ClosedPublic

Authored by andrewrk on Feb 12 2018, 2:36 PM.

Details

Summary

what do we think? is it time?

Diff Detail

Repository
rL LLVM

Event Timeline

andrewrk created this revision.Feb 12 2018, 2:36 PM
andrewrk retitled this revision from promote WebAssembly from experimental to normal target to [WebAssembly] promote from experimental to normal target.
jfo added a subscriber: jfo.Feb 12 2018, 3:01 PM

I think it's getting closer. IMO we should at least land https://reviews.llvm.org/D43147 and https://reviews.llvm.org/D40526 first, and address any follow-up issues.

There's also the question of testing and buildbots, though the way those requirements are worded in the DeveloperPolicy could be interpreted as not applying to wasm.

hans added a comment.Feb 13 2018, 1:28 AM

Whenever it lands, please include an update to docs/ReleaseNotes.rst :-)

We should probably also wait for the WebAssembly committee to settle on final instruction names.

andrewrk added a comment.EditedMar 2 2018, 9:55 AM

Would llvm interact with grow_memory or memory_size instructions? Seems like that would be the frontend or standard library's job. It's not in LLVM's business to interact with the "OS" by allocating memory.

Indeed; I don't anticipate LLVM will be using mem.grow or mem.size itself. But, clang has __builtin_wasm_mem_grow and __builtin_wasm_mem_size, to allow standard libraries to use them. It's desirable that the names of these builtins, and the corresponding LLVM IR intrinsics, match the names in the official WebAssembly documentation.

Ping. Is it time yet? Looks like the stated blockers are resolved now.

Indeed; after discussing this with a few other folks, I think this is getting close.

I'm going to refresh https://reviews.llvm.org/D40526 in hopes of landing it.

https://reviews.llvm.org/D40526 has now landed. Is there anything else we should do before leaving "experimental" status?

Is it important to resolve https://reviews.llvm.org/D43675 (Rename imported/exported memory symbol to __linear_memory) first?

Would be nice to get this CL in before we switch out of experimental: https://reviews.llvm.org/D50568
Not only does this change the text syntax for assembler and disassembler, it changes the instruction format used in MC.
As such it is a larger, messy change that would be good to have behind us, such that future changes will likely change any of these less.
I had to revert it to fix (hopefully) one last thing, hopefully re-land it soonish.

https://reviews.llvm.org/D50568 is now fixed!

This just leaves https://reviews.llvm.org/D43675. However, @echristo pointed out on llvm-dev that since LLVM just had a release, we have 6 months until they next one, which is likely enough time to resolve D43675 and get some general stabilizing in. And lots of people are eager for us to flip this switch. So let's do it!

I'll land the patch here as soon as I run a few more tests.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2018, 5:44 PM
Closed by commit rL342701: [WebAssembly] Add WebAssembly to LLVM_ALL_TARGTS. (authored by djg, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.

Woah! This a big step people! Congratulations.

On a practical note, presumably these mean that a lot more LLVM bots will be building and test wasm, right? Are there many bots that build all default targets?

Woah! This a big step people! Congratulations.

Indeed, we've come a long way!

On a practical note, presumably these mean that a lot more LLVM bots will be building and test wasm, right? Are there many bots that build all default targets?

There are a bunch, and notably, some of them run with a bunch of sanitizer configurations, and unfortunately, one of them found a memory leak. So for now I submitted r342707 to revert the change until we have a chance to fix it.