Updated waitqueue support#8672
Conversation
stevenfontanella
force-pushed
the
waitqueue
branch
10 times, most recently
from
a621ae7 to
5944f1c
Compare
stevenfontanella
force-pushed
the
waitqueue
branch
2 times, most recently
from
fbb212f to
7984294
Compare
stevenfontanella
force-pushed
the
waitqueue
branch
from
7984294 to
ec73bbe
Compare
aheejin
left a comment
aheejin
left a comment
There was a problem hiding this comment.
Sorry for the late reply!
Haven't read everything yet, but some nits and questions:
| } | ||
|
|
||
| note(&curr->ref, Type(*ht, Nullable)); | ||
| note(&curr->waitqueue, Type(HeapType::waitqueue, Nullable)); |
There was a problem hiding this comment.
The waitqueue heap type should be shared here.
|
|
||
| note(&curr->ref, Type(*ht, Nullable)); | ||
| void visitWaitqueueNotify(WaitqueueNotify* curr) { | ||
| note(&curr->waitqueue, Type(HeapType::waitqueue, Nullable)); |
| void visitWaitqueueNew(WaitqueueNew* curr) { info.note(curr->type); } | ||
| void visitWaitqueueNotify(WaitqueueNotify* curr) { | ||
| info.note(curr->waitqueue->type); | ||
| } |
There was a problem hiding this comment.
No need to have visitors for waitqueue.new and waitqueue.notify. They don't have type immediates in the binary or text formats and they don't use any defined types that we have to make sure to keep around.
| void visitArrayNewElem(ArrayNewElem* curr) { generative = true; } | ||
| void visitArrayNewFixed(ArrayNewFixed* curr) { generative = true; } | ||
| void visitContNew(ContNew* curr) { generative = true; } | ||
| void visitWaitqueueNew(WaitqueueNew* curr) { generative = true; } |
There was a problem hiding this comment.
notify should probably also be considered generative, since calling it more than once can produce different results each time. Although it looks like the linear memory notify is not considered generative... Maybe just a TODO for now, then.
| auto null = | ||
| builder.makeRefNull(HeapType(HeapType::waitqueue).getBasic(share)); | ||
| if (!type.isNullable()) { | ||
| return builder.makeRefAs(RefAsNonNull, null); |
There was a problem hiding this comment.
We should add an assertion that we're in a function context here. If we're not, for example because we are creating a global initializer, then we would generate invalid IR here.
There was a problem hiding this comment.
I didn't realize this but from looking at the other code, it looks like it's expected that we may generate invalid IR here and the caller will fix it up later: 1 2. Basically after generating ref.as_non_null (ref.null waitqueue), the calling code in setupGlobals would just change this into an MVP type. Added a comment here and updated the comment in the second code link for now.
And looking more, it seems like this may hit a bug for table initializers, because we use makeConst in them, which flows to this code (and other code in this function that can generate ref.as_non_null), and never gets fixed up like globals do. Will fix this separately.
| constexpr HeapType array = HeapType::array; | ||
| constexpr HeapType exn = HeapType::exn; | ||
| constexpr HeapType string = HeapType::string; | ||
| constexpr HeapType waitqueue = HeapType::waitqueue; |
There was a problem hiding this comment.
Maybe as a special case we should have constexpr HeapType sharedWaitqueue as well because the unshared version is so useless. OTOH, maybe consistency is more important.
3 participants
Part of #8315. Implements the updated design from https://github.com/WebAssembly/shared-everything-threads/blob/main/proposals/shared-everything-threads/Overview.md#managed-waiter-queues / WebAssembly/shared-everything-threads#110. Also see the discussion in WebAssembly/shared-everything-threads#102. This PR implements struct.wait for i32 fields only at the moment (like in V8).
Binary constants are not spec'ed yet and taken from https://chromium-review.googlesource.com/c/v8/v8/+/7790723.
Remaining after this PR:
eqref