Connie/threading proposal#57
Conversation
…o connie/threading-proposal
…ested, move the wait list to its own file
…atomic instructions (except fence)
Also includes assert_return's nan:canonical/nan:arithmetic and some of the barebones of multi-memory
…o connie/threading-proposal
ErikMcClure
left a comment
There was a problem hiding this comment.
Please double check the types being parsed from the binary wasm file.
include/innative/opcodes.h
Outdated
| OP_i64_atomic_rmw16_cmpxchg_u = 0x4D, | ||
| OP_i64_atomic_rmw32_cmpxchg_u = 0x4E, | ||
|
|
||
| OP_atomic_end, |
There was a problem hiding this comment.
I would remove OP_atomic_end and simply set OP_END equal to OP_i64_atomic_rmw32_cmpxchg_u because OP_START is kind of arbitrary as well, and that check only actually works if additional atomic instructions are contiguous, which might not be true, so there's no reason to introduce false assurance.
|
|
||
| namespace innative::atomic_details { | ||
| constexpr auto OP_START = OP_i32_atomic_load; | ||
| constexpr auto OP_END = OP_atomic_end; |
There was a problem hiding this comment.
set this to OP_i64_atomic_rmw32_cmpxchg_u
innative-env/atomics.c
Outdated
|
|
||
| #elif defined(POSIX) | ||
|
|
||
| static int64_t in_atomic_load64(int64_t* address) {} |
There was a problem hiding this comment.
In order for linux to compile at all this function will at least return something, so maybe change this to return *address and put a // TODO comment here.
innative-env/atomics.h
Outdated
| // Copyright (c)2020 Black Sphere Studios | ||
| // For conditions of distribution and use, see copyright notice in innative.h | ||
|
|
||
| #pragma once |
There was a problem hiding this comment.
Do not use #pragma once, because it won't work if the same header file is duplicated in two different locations (which can happen if a library depending on this library contains it's own set of headers since nobody can agree on how to properly deal with them). Also, GCC whines about it. Use include guards instead using the standard format.
innative/parse.cpp
Outdated
|
|
||
| IN_ERROR innative::ParseResizableLimits(Stream& s, ResizableLimits& limits) | ||
| { | ||
| // This will only actually read a single byte |
There was a problem hiding this comment.
Why is this using ParseVarUInt32 if it will only actually read a single byte? If there is only one byte it should be varuint7, unless the spec has done something very strange.
There was a problem hiding this comment.
I'm actually not sure on this one tbh. Technically according to the spec the only valid values here are {0,1,3}, but it was already reading a varuint32. I can change it to varint7 if you'd prefer though
innative/parse.cpp
Outdated
| if(err < 0) | ||
| return err; | ||
|
|
||
| ins.immediates[0]._varuint32 = alignByte & 0b111; |
There was a problem hiding this comment.
Why are we assigning a single byte to _varuint32?
There was a problem hiding this comment.
This was just for symmetry with the existing load/store instructions. They also used the _varuint32 field even though the spec says they can't be bigger than 0x7
innative/serialize.cpp
Outdated
| t.Push(WatToken{ WatTokens::INTEGER, 0, 0, 0, limits.maximum }); | ||
| if(limits.flags & WASM_LIMIT_SHARED) | ||
| t.Push(WatToken{ WatTokens::SHARED }); | ||
| // TODO: Should we serialize an UNSHARED token in the other case? |
There was a problem hiding this comment.
No, unless other wasm serializers decide to do this (but none of them support threads yet).
innative/utility.cpp
Outdated
| } | ||
|
|
||
| uint8_t GetInstruction(StringSpan ref) | ||
| uint16_t GetInstruction(StringSpan ref) |
There was a problem hiding this comment.
Once again, if this is supposed to capture all instruction bytes in a single type, this type should be defined based off of the maximum number of instruction bytes.
Implement #35
The implementation of wait/notify is far from optimized but it works for now. It can be greatly improved if the spec relaxes the requirement for no spurious failures.
imports.wast will not be able to pass until multi-memory is implemented