-
Notifications
You must be signed in to change notification settings - Fork 12
Implement MemoryFill and MemoryCopy bulk memory ops
#29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
5ca4076
59e7ffc
474460c
306b0c6
5c7c12f
ca0574c
993f4c3
b814f13
53203cb
66c7510
8af6347
3d25da0
c1fbd74
48618fb
6df7ecb
8d9b484
eae8e8a
6ef420d
979e0df
e67949d
144370e
e19b912
106003c
66d09e7
76757f7
4e19a9b
82254f1
b890cf0
846bbf2
d99970c
1cf9f32
07673a9
a9b38a4
355c6fc
96e5a29
338cdec
5373bf2
04057a0
4817a69
220b7ee
618f679
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,23 +11,69 @@ import Spec as Spec (tests) | |
|
|
||
| main :: IO () | ||
| main = do | ||
| mwasmPathMVP <- lookupEnv "WASM_SPEC_TESTS_MVP" | ||
| testDirMVP <- case mwasmPathMVP of | ||
| Nothing -> error "Please define WASM_SPEC_TESTS_MVP to point to .../WebAssembly/spec/test/core" | ||
| Just path -> pure path | ||
| mwasmPath <- lookupEnv "WASM_SPEC_TESTS" | ||
| testDir <- case mwasmPath of | ||
| Nothing -> error "Please define WASM_SPEC_TESTS to point to .../WebAssebly/spec/test/core" | ||
| Just path -> return path | ||
| Nothing -> error "Please define WASM_SPEC_TESTS to point to .../WebAssembly/spec/test/core" | ||
| Just path -> pure path | ||
| putStrLn $ "Using wasm MVP spec test directory: " ++ testDirMVP | ||
| putStrLn $ "Using wasm spec test directory: " ++ testDir | ||
|
Comment on lines
+22
to
23
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need two versions of the test suite? Can we not simply run the latest, possibly excluding new tests that we don't support?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I had to disable too many tests that won't run in the new suite, and became concerned about regressing on those. |
||
|
|
||
| filesMVP <- listDirectory testDirMVP | ||
| let wastFilesMVP = flip concatMap filesMVP $ \file -> | ||
| [ testDirMVP ++ "/" ++ file | ||
| | ".wast" `isSuffixOf` file | ||
| && file `notElem` | ||
| [ "inline-module.wast" | ||
| -- We aren't going to bother fully supporting | ||
| -- Unicode function names in the reference interpreter yet. | ||
| , "names.wast" | ||
| ] | ||
| ] | ||
|
|
||
| files <- listDirectory testDir | ||
| let wastFiles = flip concatMap files $ \file -> | ||
| [ testDir ++ "/" ++ file | ||
| | ".wast" `isSuffixOf` file | ||
| && "inline-module.wast" /= file | ||
| -- We aren't going to bother fully supporting | ||
| -- Unicode function names in the reference interpreter yet. | ||
| && "names.wast" /= file | ||
| && file `notElem` | ||
| [ "inline-module.wast" | ||
| -- We aren't going to bother fully supporting | ||
| -- Unicode function names in the reference interpreter yet. | ||
| , "names.wast" | ||
| -- These contain features that `winter` won't accept yet | ||
| , "bulk.wast" | ||
| , "memory_init.wast" | ||
| , "ref_null.wast" | ||
| , "table_get.wast" | ||
| , "table_grow.wast" | ||
| , "table.wast" | ||
| , "ref_is_null.wast" | ||
| , "ref_func.wast" | ||
| , "unreached-valid.wast" | ||
| , "exports.wast" | ||
| , "imports.wast" | ||
| , "br_table.wast" | ||
| , "table_size.wast" | ||
| , "table_fill.wast" | ||
| , "table_init.wast" | ||
| , "table_set.wast" | ||
| , "table_copy.wast" | ||
| , "global.wast" | ||
| , "linking.wast" | ||
| , "binary-leb128.wast" | ||
| , "elem.wast" | ||
| , "call_indirect.wast" | ||
| , "binary.wast" | ||
| , "select.wast" | ||
| ] | ||
| ] | ||
|
|
||
| defaultMain $ testGroup "main" | ||
| [ Property.tests | ||
| , Unit.tests | ||
| , Spec.tests wastFiles | ||
| , Spec.tests [] "spec MVP" wastFilesMVP | ||
| , Spec.tests ["--enable-bulk-memory"] "spec" wastFiles | ||
| ] | ||
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks quite different from the reference code in
https://github.com/WebAssembly/spec/blob/ffb5e3b40db7a45237d4e2fc972c1552ac4abef5/interpreter/exec/eval.ml#L413-L428
(and likewise below)
The design of
winteris to be a pretty much direct port, so that chances are good to get the semantics right, and also to be able to follow and apply subsequent changes easily.We have occasionally deviated, for performance reasons (e.g. https://www.joachim-breitner.de/blog/765-Faster_Winter_7__The_Zipper). Is this the reason here? Maybe worth adding a comment here… hmm, but I see you aren’t still implementing this to byte-wise operations here.
So why not just follow the logic of the reference code directly?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I didn't even look at the other reference :-)
Will do.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at that reference implementation makes me think it is written with proof assistants in mind. It lowers the complex instructions to simple ones and stucturally "smaller" identical ones. There is a case of
MemoryCopythat isn't even tail recursive. So to make this remotely performant we'll have to rewrite these anyway. Otherwise even the Rust-style "do prefix and suffix bytewise and the inner partu64-wise" will be executing faster.