Merge wide-arithmetic proposal into the specification - #2224
Merge wide-arithmetic proposal into the specification#2224alexcrichton wants to merge 92 commits into
Conversation
No actual content just yet, just placeholders for future content.
Add a skeleton updated README/Overview
This is mostly a transcription of the presentation we made to the CG this week. I've also taken the time to flesh out a description of the current state of WebAssembly at least relative to Wasmtime with some links and words.
Fill out an initial Overview.md
Hard tabs snuck in by accident.
Replace hard tabs with spaces in overview
This commit fills out the overflow flags alternative in the
`Overview.md` with some more analysis and discussion on the alternative
relative to `i64.{add,sub}128` for 128-bit arithmetic.
chore: fix README typo
Fill out the alternatives section for overflow flags
Some recent [benchmarking] had a surprising result I wasn't trying to dig for. Notably as summarized in #11 some more use cases of widening multiplication being more optimal than 128-by-128 bit multiplication have started to arise. Coupled with local benchmarking confirming that both on x64 and aarch64 that widening multiplication has more support in LLVM for more optimal lowerings and was easier to implement in Wasmtime than the 128-by-128 bit multiplication once various optimizations were implemented. In the end `i64.mul128`, which was primarily motivated by "feels cleaner" and "should have the same performance" as widening multiplication, does not appear to have the expected performance/implementation tradeoff. Getting an as-performant `i64.mul128` instruction relative to `i64.mul_wide_{s,u}` has required more work than expected and so the balance of concerns has me now tipping away from `i64.mul128`, despite it being "less clean" compared to the add/sub opcodes proposed in this PR. Closes #11 [benchmarking]: #6 (comment)
This summarizes findings and discussion in #4 into the overview with a section that explains why comparison-related instructions are omitted.
Add some links to issue discussion from the overview
Document leaving out comparison-related instructions
Remove `i64.mul128`, add `i64.mul_wide_{s,u}`
Summarize #5 and write down some words for this in the overview.
This fills out some more in the overview about the alternative for overflow flags. I've expanded the section about how the naive wasm instructions for overflow/add-with-carry are significantly different than hardware to help further explain the complications they cause. I've also noted a "way out there" alternatives to possibly consider.
Try to more concretely talk about `i1` and "machine state" and all the complexities that arise.
Summarize discussion of shifting operators in Overview
Fill out some more rationale on overflow flags
As voted on in today's meeting the name of the proposal is now "wide-arithmetic" instead of "128-bit-arithmetic" (apologies for now-broken links that are going to be generated...)
Rename repository to "wide-arithmetic"
Landed and/or made PRs in a number of repos after phase 2 today.
Update implementation status
Merge upstream main branch
Merge in the latest spec
Merge with main
Another merge with main
Now that the rebased spec is rendered
Update README links
Add a diff-to-main link
Add chasm to supported engine list
Add wasm-language-tools to list of implementations
JSC added support in https://commits.webkit.org/311366@main for wide arithmetic behind a flag (useWasmWideArithmetic).
Add JavaScriptCore to implementations
Add SpiderMonkey implementation
include Wasmer implementation link
# Conflicts: # .github/workflows/ci-interpreter.yml # spectec/test-frontend/TEST.md # spectec/test-interpreter/TEST.md # spectec/test-latex/.gitignore # spectec/test-latex/TEST.md # spectec/test-middlend/TEST.md # spectec/test-splice/TEST.md
Merge upstream spec
| ../test-interpreter/spec-test-3/wide-arithmetic.wast:25.1-28.44 | ||
| - Test failed at ../test-interpreter/spec-test-3/wide-arithmetic.wast:25.1-28.44 (Algorithm not found: WIDEOP (interpreting CaseV(WIDEOP, [CaseV(I64, []), CaseV(ADD128, [])]) at )) | ||
| Test failed for ../test-interpreter/spec-test-3/wide-arithmetic.wast |
There was a problem hiding this comment.
I believe this shows the meta-interpreter failing. We probably want to fix that before merging (although the meta-interpreter is not formally part of the standards process).
There was a problem hiding this comment.
I might need some help on this one and the other failure in CI as well. One failure on CI is this one which asserts that wasm-3.0 is the same as wasm-latest where I believe that this PR is the first divergence from that.
This failure I believe is due to the fact that spectec/test-interpreter/spec-test-3 is a symlink to ../../test/core which is also no longer valid since this is the first proposal to go beyond Wasm 3.0.
Should I disable the "ensure-wasm-latest" CI check? Should I copy the testsuite before-this-PR to spec-test-3 in what looks like a similar manner of how spec-test-{1,2} were copied?
There was a problem hiding this comment.
Should I disable the "ensure-wasm-latest" CI check?
I guess if we don't intend wide-arithmetic to be part of Wasm 3.0, then we will want to create a new wasm-3.1 (or wasm-4.0???) directory that can start tracking wasm-latest instead. I'm not sure what @rossberg's plan is for the numbering, though.
The new directory will have to be created by copying the current wasm-latest directory while using a merge commit to preserve its history, then this merge will have to update both wasm-latest and the new wasm-3.1 (or whatever) directory identically.
(The goal of all this is to have all proposals update just wasm-latest so merges to or from the main spec repo apply changes to a single canonical location, while also having versioned directories with meaningful histories.)
Should I copy the testsuite before-this-PR to spec-test-3 in what looks like a similar manner of how spec-test-{1,2} were copied?
Yes, that sounds like it's probably the expected workflow. I'm not sure what the expectations are around backporting new tests to these snapshots, but that doesn't directly affect this merge.
9 participants
This commit is an attempt of mine to merge the wide-arithmetic proposal into the main specification here. I'm not actually entirely sure if this is something I should do or defer to someone else, but I figure I can at least make a skeleton here! I've done a merge from the spec's current status back into wide-arithmetic, sorted out things like CI and such, and now this is merging back into the main specification. This is intended to preserve the git history but I can also squash thing down if that's additionally desired.
I'll note that I think all the spec changes are largely unreviewed, so I'm happy to adjust things as necessary. I'd request special attention to the
TEST.mdfiles as I suspect I messed up something there, but my belief is that this should otherwise largely be complete.