This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Switch to a more traditional assembly syntax
AbandonedPublic

Authored by sunfish on Sep 23 2015, 3:07 PM.

Details

Reviewers
jfb
Summary

The s-expression syntax is problematic for MC because it requires nested instructions, nested control flow, and entirely custom handling of static initializers. Instead of using s-expressions as the immediate output of the compiler, use a more traditional-looking assembly syntax using a stack-machine-like mechanism to represent nesting.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish updated this revision to Diff 35558.Sep 23 2015, 3:07 PM
sunfish retitled this revision from to [WebAssembly] Switch to a more traditional assembly syntax.
sunfish updated this object.
sunfish added a reviewer: jfb.
sunfish set the repository for this revision to rL LLVM.
sunfish added a subscriber: llvm-commits.
jfb edited edge metadata.Sep 23 2015, 3:28 PM

I haven't reviewed the patch itself yet, but I'd like to bring up an issue: we definitely want to do this move for the reasons you outline (as we've discussed offline), but I think we should first get the code support further along so we can get an end-to-end prototype working using s-epxressions first. Once we have this, then browsers can play with a specific version of LLVM while we fix the bigger issues that you outline.

Do you think we could first finalize control flow and linear memory initialization, snapshot things, and then apply this patch?

sunfish updated this revision to Diff 36428.Oct 2 2015, 7:57 PM
sunfish edited edge metadata.

Rebased, and shrunk with parts of this patch being checked in independently.

jfb added a comment.Oct 5 2015, 1:12 PM

This looks great. Only a few minor things, otherwise lgtm.

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
122

Comment that wasm arguments and locals are in the same number space, with args coming first (and there are no vaargs).

173–174

Is this check needed? It seems like the below code does the right thing without it.

211

Should we just drop GLOBAL since it's not in the design anymore?

238

Move NeedComma = true; to here?

239

Create a new string here instead?

242

@pop is a bit confusing since it's more of an internal opcode not an identifier (which we use @ for). Maybe %pop or something (since we're already using $ and # looks like a comment)?

sunfish marked 6 inline comments as done.Oct 5 2015, 2:54 PM
sunfish added inline comments.
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
173–174

EmitRawText automatically appends a newline, so this avoids printing an extra newline in that case.

211

We need to keep global addresses symbolic until link time. So even if wasm itself doesn't have GLOBAL, we need something like it at this stage.

242

How about just pop?

sunfish updated this revision to Diff 36563.Oct 5 2015, 2:54 PM
sunfish marked 3 inline comments as done.

Update for review comments.

jfb added inline comments.Oct 5 2015, 3:01 PM
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
242

sgtm

sunfish abandoned this revision.Oct 5 2015, 5:30 PM

Commited in r249364. Thanks!