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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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?
This looks great. Only a few minor things, otherwise lgtm.
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp | ||
---|---|---|
127 | Comment that wasm arguments and locals are in the same number space, with args coming first (and there are no vaargs). | |
193โ194 | Is this check needed? It seems like the below code does the right thing without it. | |
232 | Should we just drop GLOBAL since it's not in the design anymore? | |
259 | Move NeedComma = true; to here? | |
289 | Create a new string here instead? | |
292 | @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)? |
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp | ||
---|---|---|
193โ194 | EmitRawText automatically appends a newline, so this avoids printing an extra newline in that case. | |
232 | 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. | |
292 | How about just pop? |
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp | ||
---|---|---|
292 | sgtm |
Comment that wasm arguments and locals are in the same number space, with args coming first (and there are no vaargs).