[wasm][shared] Add waitqueue type and get/set [v8/v8 : main]
Manos Koukoutos (Gerrit)
Manos Koukoutos added 1 comment![]( "Open in Gerrit")
Matthias, PTAL overall.
Jakob, please approve src/debug.
Dominik, please approve src/profiler.
Related details
- Dominik Inführ
- Jakob Kummerow
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
src/profiler LGTM but I am not an owner, adding @cbruni instead.
Related details
- Camillo Bruni
- Jakob Kummerow
- Manos Koukoutos
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camillo Bruni (Gerrit)
Camillo Bruni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Camillo Bruni
| Code-Review | +1 |
1 comment
- Jakob Kummerow
- Manos Koukoutos
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 16 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
16 comments
LGTM with nits (accidentally did a full review).
imm.array_type->element_type().Unpacked());`element_type`
imm.array_type->element_type().name().c_str());`element_type`
Value* value = Push(imm.array_type->element_type().Unpacked());`element_type`
imm.index.index, imm.array_type->element_type().name().c_str());`element_type`
struct_type->field(field.field_imm.index).Unpacked());`field_type`
imm.index.index, imm.array_type->element_type().name().c_str());`element_type`
consume_bytes(1, " waitqueue", tracer_);Technically this is unnecessary, but it _might_ improve the debugging experience, and we'll eventually drop this entire `if`-block anyway, so it's fine.
kWaitQueueCode = 0x5c, // -0x21, packed type.`0x5c == -0x24`, `0x5f == -0x21`. Which is it?
Might also be nice to put this into the section for non-finalized proposals below.
// TODO(manoskouk): The spec now defines kWaitQueueCode as 0x64 which is the`0x68`
case wasm::kWaitQueue:Why is waitqueue handled like an `i64` here? I would have expected the same `UNREACHABLE()` as i8/i16 since exceptions can't pass around packed field values. (And in places that can handle them, the same handling as i32 would make more sense.)
let struct = builder.addStruct([makeField(kWasmWaitQueue, true)],
kNoSuperType, true, true);Prefer the new syntax:
```
addStruct({fields: [makeField(kWasmWaitQueue, true)], shared: true});
```
// TODO(manoskouk): The spec now defines this as 0x64 which is the same as`0x68`
// struct.get_s/u failIf you added the new failure test here, that'd be faster than the mjsunit version.
Related details
- Manos Koukoutos
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Manos Koukoutos added 13 comments![]( "Open in Gerrit")
imm.array_type->element_type().Unpacked());Manos Koukoutos`element_type`
Done
imm.array_type->element_type().name().c_str());Manos Koukoutos`element_type`
Done
Value* value = Push(imm.array_type->element_type().Unpacked());Manos Koukoutos`element_type`
Done
imm.index.index, imm.array_type->element_type().name().c_str());Manos Koukoutos`element_type`
Done
struct_type->field(field.field_imm.index).Unpacked());Manos Koukoutos`field_type`
Done
imm.index.index, imm.array_type->element_type().name().c_str());Manos Koukoutos`element_type`
Done
`0x5c == -0x24`, `0x5f == -0x21`. Which is it?
Might also be nice to put this into the section for non-finalized proposals below.
Done
// TODO(manoskouk): The spec now defines kWaitQueueCode as 0x64 which is theManos Koukoutos`0x68`
Done
Why is waitqueue handled like an `i64` here? I would have expected the same `UNREACHABLE()` as i8/i16 since exceptions can't pass around packed field values. (And in places that can handle them, the same handling as i32 would make more sense.)
Done
let struct = builder.addStruct([makeField(kWasmWaitQueue, true)],
kNoSuperType, true, true);Prefer the new syntax:
```
addStruct({fields: [makeField(kWasmWaitQueue, true)], shared: true});
```
Done
kNoSuperType, true, true);Manos Koukoutossame here
Done
// TODO(manoskouk): The spec now defines this as 0x64 which is the same asManos Koukoutos`0x68`
Done
If you added the new failure test here, that'd be faster than the mjsunit version.
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Manos Koukoutos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Manos Koukoutos
| Commit-Queue | +2 |
1 comment
Since Matthias is away for a few more days, I am landing this with Jakob's review.
Related details
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: test/mjsunit/wasm/wasm-module-builder.js
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: test/mjsunit/wasm/shared-everything/wait-queue.js
Insertions: 2, Deletions: 19.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/wasm/wasm-objects.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: test/unittests/wasm/function-body-decoder-unittest.cc
Insertions: 20, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/wasm/wasm-constants.h
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/wasm/function-body-decoder-impl.h
Insertions: 5, Deletions: 5.
The diff is too large to show. Please review the diff.
```
Change information
[wasm][shared] Add waitqueue type and get/set
With this CL, a waitqueue is represented as a plain i32 packed type (see
https://github.com/WebAssembly/shared-everything-threads/issues/102).
Arrays of waitqueues will not be supported for now.
- M src/compiler/turboshaft/operations.cc
- M src/compiler/turboshaft/operations.h
- M src/compiler/turboshaft/wasm-in-js-inlining-reducer-inl.h
- M src/compiler/turboshaft/wasm-lowering-reducer.h
- M src/debug/debug-wasm-objects.cc
- M src/diagnostics/objects-printer.cc
- M src/profiler/heap-snapshot-generator.cc
- M src/wasm/baseline/arm/liftoff-assembler-arm-inl.h
- M src/wasm/baseline/ia32/liftoff-assembler-ia32-inl.h
- M src/wasm/baseline/liftoff-compiler.cc
- M src/wasm/baseline/liftoff-register.h
- M src/wasm/c-api.cc
- M src/wasm/constant-expression-interface.cc
- M src/wasm/function-body-decoder-impl.h
- M src/wasm/fuzzing/random-module-generation.cc
- M src/wasm/module-decoder-impl.h
- M src/wasm/module-instantiate.cc
- M src/wasm/turboshaft-graph-interface-inl.h
- M src/wasm/turboshaft-graph-interface.cc
- M src/wasm/value-type.h
- M src/wasm/wasm-constants.h
- M src/wasm/wasm-debug.cc
- M src/wasm/wasm-external-refs.cc
- M src/wasm/wasm-init-expr.h
- M src/wasm/wasm-js.cc
- M src/wasm/wasm-objects-inl.h
- M src/wasm/wasm-objects.cc
- M src/wasm/wasm-value.h
- M src/wasm/wrappers-inl.h
- M src/wasm/wrappers.h
- M test/common/wasm/wasm-module-runner.cc
- A test/mjsunit/wasm/shared-everything/wait-queue.js
- M test/mjsunit/wasm/wasm-module-builder.js
- M test/unittests/wasm/function-body-decoder-unittest.cc
- M test/unittests/wasm/subtyping-unittest.cc
- M tools/wasm/mjsunit-module-disassembler-impl.h
Code-Review: +1 by Camillo Bruni, +1 by Jakob Kummerow
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Hi Manos, I've noticed this CL isn't making changes to arm64/x64 liftoff files and only changes 32-bit platforms. Does this need to be ported to ppc64/s390x? We also don't seem to be hitting any `UNREACHABLE` blocks.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Manos Koukoutos added 1 comment![]( "Open in Gerrit")
Hi Manos, I've noticed this CL isn't making changes to arm64/x64 liftoff files and only changes 32-bit platforms. Does this need to be ported to ppc64/s390x? We also don't seem to be hitting any `UNREACHABLE` blocks.
I would suggest you check for switch statements over `ValueKind`, if any, and complete them accordingly. That said, nothing might be required if all tests pass.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Manos KoukoutosHi Manos, I've noticed this CL isn't making changes to arm64/x64 liftoff files and only changes 32-bit platforms. Does this need to be ported to ppc64/s390x? We also don't seem to be hitting any `UNREACHABLE` blocks.
I would suggest you check for switch statements over `ValueKind`, if any, and complete them accordingly. That said, nothing might be required if all tests pass.
Thanks but can I ask why arm needs to explicitly handle kWaitQueue but arm64 doesn't?
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Manos Koukoutos added 1 comment![]( "Open in Gerrit")
Manos KoukoutosHi Manos, I've noticed this CL isn't making changes to arm64/x64 liftoff files and only changes 32-bit platforms. Does this need to be ported to ppc64/s390x? We also don't seem to be hitting any `UNREACHABLE` blocks.
Milad FarazmandI would suggest you check for switch statements over `ValueKind`, if any, and complete them accordingly. That said, nothing might be required if all tests pass.
Thanks but can I ask why arm needs to explicitly handle kWaitQueue but arm64 doesn't?
I think you are right, we do not need to handle it explicitly in arm either.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Manos KoukoutosHi Manos, I've noticed this CL isn't making changes to arm64/x64 liftoff files and only changes 32-bit platforms. Does this need to be ported to ppc64/s390x? We also don't seem to be hitting any `UNREACHABLE` blocks.
Milad FarazmandI would suggest you check for switch statements over `ValueKind`, if any, and complete them accordingly. That said, nothing might be required if all tests pass.
Manos KoukoutosThanks but can I ask why arm needs to explicitly handle kWaitQueue but arm64 doesn't?
I think you are right, we do not need to handle it explicitly in arm either.
Thank you for confirming.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |