Implement switch instruction. [v8/v8 : main]
Francis McCabe (Gerrit)
Francis McCabe removed Jakob Linke from this change![]( "Open in Gerrit")
- Jakob Linke
Related details
- Deepti Gandluri
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis McCabe (Gerrit)
Francis McCabe added 1 comment![]( "Open in Gerrit")
This is a large-ish CL. However, the largest part is actually test files.
Related details
- Nico Hartmann
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 7 comments![]( "Open in Gerrit")
Just a few small comments as I was passing by ;)
Implement switch instruction.nit: add a tag (`[wasm]`? `[wasmfx]`?)
nit: can you maybe add a link to the spec of to whatever documentation explains what this switch instruction is?
OpIndex loaded = this->Asm().LoadOffHeap(Use a `V<>` type if possible.
OpIndex loaded = this->Asm().LoadOffHeap(I don't particularly like this variable name; any chance you can make it describe a bit better what it contains?
(same thing later in this file)
this->Asm().StoreOffHeap(arg_buffer, args[index].op,Doesn't `__` work?
Related details
- Francis McCabe
- Jakob Kummerow
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis McCabe (Gerrit)
Francis McCabe added 6 comments![]( "Open in Gerrit")
nit: add a tag (`[wasm]`? `[wasmfx]`?)
Acknowledged
nit: can you maybe add a link to the spec of to whatever documentation explains what this switch instruction is?
Acknowledged
OpIndex loaded = this->Asm().LoadOffHeap(I don't particularly like this variable name; any chance you can make it describe a bit better what it contains?
(same thing later in this file)
Agreed. Slightly better name: ith_return.
OpIndex loaded = this->Asm().LoadOffHeap(Use a `V<>` type if possible.
This is not done anywhere else for OpIndex values.
this->Asm().StoreOffHeap(arg_buffer, args[index].op,Francis McCabeDoesn't `__` work?
Unfortunately not. This code is in a lambda. __ XX expands to Asm().XX where we need this->Asm().XX.
this->Asm().StoreOffHeap(Francis McCabesame
See above.
Related details
- Darius Mercadier
- Jakob Kummerow
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 2 comments![]( "Open in Gerrit")
OpIndex loaded = this->Asm().LoadOffHeap(Use a `V<>` type if possible.
This is not done anywhere else for OpIndex values.
This file is full of `V<>` types. In fact, `OpIndex` appears around 200 times and `V<>` appears 700 times!
If this specific OpIndex is hard express as `V<>`, that's ok, but please consider doing it. At least `V<Any>` looks like it should work. If the value is guaranteed to be tagged or untagged, you can use `V<Object>` or `V<Untagged>`.
OpIndex loaded = this->Asm().LoadOffHeap(Francis McCabeI don't particularly like this variable name; any chance you can make it describe a bit better what it contains?
(same thing later in this file)
Agreed. Slightly better name: ith_return.
- Francis McCabe
- Jakob Kummerow
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 7 comments![]( "Open in Gerrit")
memset(stack->jmpbuf(), 0, sizeof(wasm::JumpBuffer));
stack->jmpbuf()->state = wasm::JumpBuffer::Active;
stack->jmpbuf()->stack_limit = stack->jslimit();
stack->jmpbuf()->parent = nullptr;
stack->jmpbuf()->is_on_central_stack = true;Why is this needed?
The jump buffer of the active stack is usually considered to be garbage since the SP, FP, etc. are currently changing. The jump buffer is initialized when we switch to a different stack.
Same thing for the central stack pointers below. They should only be needed after having switched stacks, at which point they should have already been initialized.
OpIndex loaded = this->Asm().LoadOffHeap(Francis McCabeUse a `V<>` type if possible.
Darius MercadierThis is not done anywhere else for OpIndex values.
This file is full of `V<>` types. In fact, `OpIndex` appears around 200 times and `V<>` appears 700 times!
If this specific OpIndex is hard express as `V<>`, that's ok, but please consider doing it. At least `V<Any>` looks like it should work. If the value is guaranteed to be tagged or untagged, you can use `V<Object>` or `V<Untagged>`.
I think it will have to be `Any` at best. The type depends on the `return_types` vector, so to have a more precise `V<>` we would have to do a switch on the `return_types[index]`, where each case does the same load but with a `V<>` that matches the wasm type.
It's not worth it, especially since we just end up assigning it to `returns[index].op` which is an `OpIndex`.
sig->parameters()[sig->parameters().size() - 1].ref_index());```suggestion
sig->parameters().last().ref_index());
```
V<WasmContinuationObject> allocateContinuation(FullDecoder* decoder) {```suggestion
V<WasmContinuationObject> AllocateContinuation(FullDecoder* decoder) {
```
// The external pointer is initialized in the {suspend_wasmfx_stack} C call.Obsolete comment.
if (!to) return kNullAddress;I think we prefer to spell it out as `to == nullptr`.
if (v8_flags.trace_wasm_stack_switching) {
PrintF("Switch from stack %d to %d\n", from->id(), target_stack->id());
}Can you move this after the early return? I would prefer to only print this if we did find a handler and performed the switch.
In fact you can merge it with the other trace below, where you print the handler stack.
Related details
- Francis McCabe
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
OpIndex loaded = this->Asm().LoadOffHeap(Francis McCabeUse a `V<>` type if possible.
Darius MercadierThis is not done anywhere else for OpIndex values.
Thibaud MichaudThis file is full of `V<>` types. In fact, `OpIndex` appears around 200 times and `V<>` appears 700 times!
If this specific OpIndex is hard express as `V<>`, that's ok, but please consider doing it. At least `V<Any>` looks like it should work. If the value is guaranteed to be tagged or untagged, you can use `V<Object>` or `V<Untagged>`.
I think it will have to be `Any` at best. The type depends on the `return_types` vector, so to have a more precise `V<>` we would have to do a switch on the `return_types[index]`, where each case does the same load but with a `V<>` that matches the wasm type.
It's not worth it, especially since we just end up assigning it to `returns[index].op` which is an `OpIndex`.
I'd argue that `V<Any>` is better than `OpIndex`, because `OpIndex` is implicitely convertible to any `V<>` but `V<Any>` isn't. Currently the value is immediatly passed to AnnotateResultIfReference so it doesn't make a difference, but if AnnotateResultIfReference was for instance ever updated with wrong assumptions on its input type (encoded via a proper `V<>` input rather than its current `OpIndex`), then we'd get a compile error with `V<Any>` and not with `OpIndex`.
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
OpIndex loaded = this->Asm().LoadOffHeap(Francis McCabeUse a `V<>` type if possible.
Darius MercadierThis is not done anywhere else for OpIndex values.
Thibaud MichaudThis file is full of `V<>` types. In fact, `OpIndex` appears around 200 times and `V<>` appears 700 times!
If this specific OpIndex is hard express as `V<>`, that's ok, but please consider doing it. At least `V<Any>` looks like it should work. If the value is guaranteed to be tagged or untagged, you can use `V<Object>` or `V<Untagged>`.
Darius MercadierI think it will have to be `Any` at best. The type depends on the `return_types` vector, so to have a more precise `V<>` we would have to do a switch on the `return_types[index]`, where each case does the same load but with a `V<>` that matches the wasm type.
It's not worth it, especially since we just end up assigning it to `returns[index].op` which is an `OpIndex`.
I'd argue that `V<Any>` is better than `OpIndex`, because `OpIndex` is implicitely convertible to any `V<>` but `V<Any>` isn't. Currently the value is immediatly passed to AnnotateResultIfReference so it doesn't make a difference, but if AnnotateResultIfReference was for instance ever updated with wrong assumptions on its input type (encoded via a proper `V<>` input rather than its current `OpIndex`), then we'd get a compile error with `V<Any>` and not with `OpIndex`.
Right, I'm only arguing that assigning something more precise than `Any` would require splitting the load into multiple cases and does not seem worth it. But `V<Any>` sgtm.
Jakob Kummerow (Gerrit)
Jakob Kummerow added 15 comments![]( "Open in Gerrit")
__ Mov(x8, arg_buffer_reg);
arg_buffer_reg = x8;Can't you just do `__ Mov(kCArgRegs[7], arg_buffer_reg)` up front?
this->Asm().StoreOffHeap(While you're here: `__ `
OpIndex ith_return = this->Asm().LoadOffHeap(`__ `
__ LoadOffHeap(result_buffer, offset,As you can see here, `__` works just fine inside a lambda. Please keep using it, also in the new code.
this->Asm().StoreOffHeap(arg_buffer, args[index].op,`__ `
OpIndex loaded = this->Asm().LoadOffHeap(`__ `
IF (is_on_central_stack) {
__ WasmCallRuntime(__ phase_zone(), Runtime::kThrowWasmFXSuspendError, {},Any reason this isn't a `__ TrapIf(...)` call?
this->Asm().StoreOffHeap(arg_buffer, args[index].op,`__ `, again below
__ WordPtrConstant(reinterpret_cast<uintptr_t>(return_csig))},Embedding a pointer constant makes this code uncacheable, but we cache and re-use optimized code across processes. You'll have to embed type index instead; we have relocation support canonical type indices via `__ RelocatableWasmCanonicalSignatureId(sig_index);` (or you can embed the module-specific signature index, but then you'll need to pass the `WasmTrustedInstanceData` along to the C++ function to be able to resolve it to a canonical index).
__ WordPtrConstant(reinterpret_cast<uintptr_t>(csig))},Same problem here. This needs to be fixed.
const CanonicalSig* sig) {Matching my comment in the graph builder: both here and in `switch_...` below, you'll have to take the signature index.
let sig_coro_idx = builder.types.length;`nextTypeIndex()`.
And the preferred pattern is to use that only once, e.g.:
```
let type2 = builder.nextTypeIndex() + 1;
let type1 = builder.addType(..., type2, ...);
/* type2 */ builder.addCont(type1);
```
let tag_switch = builder.addTag(makeSig([], [kWasmI32]));`kSig_i_v` is predefined
.addBody([kExprLocalGet, 0, kExprDrop]).exportFunc();Technically, for a "nop" function, you could also use `kExprNop`, or just an empty body. (You still need to call `.addBody([])` though in order to get the required `kExprEnd`.)
.addLocals(wasmRefType(cont_v_c), 1)This doesn't seem useful? If its purpose is to test some code path, a comment would be nice.
Related details
- Francis McCabe
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis McCabe (Gerrit)
Francis McCabe added 23 comments![]( "Open in Gerrit")
Picked up on key suggestions. The change incorporates changes in nearby code not originally part o cl that had the same issues.
Can't you just do `__ Mov(kCArgRegs[7], arg_buffer_reg)` up front?
I have shuffled the register loads around.
memset(stack->jmpbuf(), 0, sizeof(wasm::JumpBuffer));
stack->jmpbuf()->state = wasm::JumpBuffer::Active;
stack->jmpbuf()->stack_limit = stack->jslimit();
stack->jmpbuf()->parent = nullptr;
stack->jmpbuf()->is_on_central_stack = true;Why is this needed?
The jump buffer of the active stack is usually considered to be garbage since the SP, FP, etc. are currently changing. The jump buffer is initialized when we switch to a different stack.
Same thing for the central stack pointers below. They should only be needed after having switched stacks, at which point they should have already been initialized.
In my defense, this was suggested by Gemini.
However, if I dont zero out the jmpbuf, v8 does not build.
While you're here: `__ `
Acknowledged
Francis McCabeUse a `V<>` type if possible.
Darius MercadierThis is not done anywhere else for OpIndex values.
Thibaud MichaudThis file is full of `V<>` types. In fact, `OpIndex` appears around 200 times and `V<>` appears 700 times!
If this specific OpIndex is hard express as `V<>`, that's ok, but please consider doing it. At least `V<Any>` looks like it should work. If the value is guaranteed to be tagged or untagged, you can use `V<Object>` or `V<Untagged>`.
Darius MercadierI think it will have to be `Any` at best. The type depends on the `return_types` vector, so to have a more precise `V<>` we would have to do a switch on the `return_types[index]`, where each case does the same load but with a `V<>` that matches the wasm type.
It's not worth it, especially since we just end up assigning it to `returns[index].op` which is an `OpIndex`.
Thibaud MichaudI'd argue that `V<Any>` is better than `OpIndex`, because `OpIndex` is implicitely convertible to any `V<>` but `V<Any>` isn't. Currently the value is immediatly passed to AnnotateResultIfReference so it doesn't make a difference, but if AnnotateResultIfReference was for instance ever updated with wrong assumptions on its input type (encoded via a proper `V<>` input rather than its current `OpIndex`), then we'd get a compile error with `V<Any>` and not with `OpIndex`.
Right, I'm only arguing that assigning something more precise than `Any` would require splitting the load into multiple cases and does not seem worth it. But `V<Any>` sgtm.
I have converted it to a V<Any>
OpIndex ith_return = this->Asm().LoadOffHeap(Francis McCabe`__ `
Acknowledged
__ LoadOffHeap(result_buffer, offset,As you can see here, `__` works just fine inside a lambda. Please keep using it, also in the new code.
Acknowledged
this->Asm().StoreOffHeap(arg_buffer, args[index].op,Francis McCabe`__ `
Acknowledged
OpIndex loaded = this->Asm().LoadOffHeap(Francis McCabe`__ `
Acknowledged
IF (is_on_central_stack) {
__ WasmCallRuntime(__ phase_zone(), Runtime::kThrowWasmFXSuspendError, {},Any reason this isn't a `__ TrapIf(...)` call?
Yes, it should be.
The wasmfx explainer is actually silent on this; but, in keeping with other specs, we should trap. I have also adjusted the Suspend implementation.
this->Asm().StoreOffHeap(arg_buffer, args[index].op,Francis McCabe`__ `, again below
Acknowledged
sig->parameters()[sig->parameters().size() - 1].ref_index());Francis McCabe```suggestion
sig->parameters().last().ref_index());
```
Acknowledged
__ WordPtrConstant(reinterpret_cast<uintptr_t>(return_csig))},Embedding a pointer constant makes this code uncacheable, but we cache and re-use optimized code across processes. You'll have to embed type index instead; we have relocation support canonical type indices via `__ RelocatableWasmCanonicalSignatureId(sig_index);` (or you can embed the module-specific signature index, but then you'll need to pass the `WasmTrustedInstanceData` along to the C++ function to be able to resolve it to a canonical index).
Acknowledged
V<WasmContinuationObject> allocateContinuation(FullDecoder* decoder) {```suggestion
V<WasmContinuationObject> AllocateContinuation(FullDecoder* decoder) {
```
Acknowledged
// The external pointer is initialized in the {suspend_wasmfx_stack} C call.Francis McCabeObsolete comment.
Acknowledged
__ WordPtrConstant(reinterpret_cast<uintptr_t>(csig))},Same problem here. This needs to be fixed.
Acknowledged
Matching my comment in the graph builder: both here and in `switch_...` below, you'll have to take the signature index.
Acknowledged
I think we prefer to spell it out as `to == nullptr`.
Acknowledged
if (v8_flags.trace_wasm_stack_switching) {
PrintF("Switch from stack %d to %d\n", from->id(), target_stack->id());
}Can you move this after the early return? I would prefer to only print this if we did find a handler and performed the switch.
In fact you can merge it with the other trace below, where you print the handler stack.
Done. It was originally there as an aid to debugging.
`nextTypeIndex()`.
And the preferred pattern is to use that only once, e.g.:
```
let type2 = builder.nextTypeIndex() + 1;
let type1 = builder.addType(..., type2, ...);
/* type2 */ builder.addCont(type1);
```
I have done this. However, I dispute that this way of doing it is clearer.
let tag_switch = builder.addTag(makeSig([], [kWasmI32]));Francis McCabe`kSig_i_v` is predefined
Acknowledged
.addBody([kExprLocalGet, 0, kExprDrop]).exportFunc();Technically, for a "nop" function, you could also use `kExprNop`, or just an empty body. (You still need to call `.addBody([])` though in order to get the required `kExprEnd`.)
Acknowledged
This doesn't seem useful? If its purpose is to test some code path, a comment would be nice.
- Darius Mercadier
- Jakob Kummerow
- Thibaud Michaud
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 Code-Review+1![]( "Open in Gerrit")
- Darius Mercadier
- Francis McCabe
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 3 comments![]( "Open in Gerrit")
IF (is_on_central_stack) {
__ WasmCallRuntime(__ phase_zone(), Runtime::kThrowWasmFXSuspendError, {},Francis McCabeAny reason this isn't a `__ TrapIf(...)` call?
Yes, it should be.
The wasmfx explainer is actually silent on this; but, in keeping with other specs, we should trap. I have also adjusted the Suspend implementation.
`WebAssembly.SuspendError` was introduced for JSPI based on a user request to make suspension errors catchable. I could imagine that this is similarly useful for core stack switching? Since handlers are dynamically scoped, applications might not always be able to ensure that there is a handler for the next suspend or switch instruction.
In any case, the current implementation is inconsistent: the builtin still throws `SuspendError` if we don't find a handler. See the `TestSuspendError` test: we throw a `RuntimeError` in the first case, and `SuspendError` in the other cases.
kSimdPrefix, 0x11, // i32x4.splat```suggestion
kSimdPrefix, kExprI32x4Splat,
```
if (e.message.includes('SIMD unsupported')) {I would suggest moving this function to `stack-switching-params.js`. We already test passing SIMD parameters for resume/suspend there, and the test is skipped on no-simd configs (see mjsunit.status) so we don't need this try/catch.
Related details
- Darius Mercadier
- Francis McCabe
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis McCabe (Gerrit)
Francis McCabe added 4 comments![]( "Open in Gerrit")
A couple of things, mainly the handling of failing searches, should be resolved in a follow-up CL.
IF (is_on_central_stack) {
__ WasmCallRuntime(__ phase_zone(), Runtime::kThrowWasmFXSuspendError, {},Francis McCabeAny reason this isn't a `__ TrapIf(...)` call?
Thibaud MichaudYes, it should be.
The wasmfx explainer is actually silent on this; but, in keeping with other specs, we should trap. I have also adjusted the Suspend implementation.
`WebAssembly.SuspendError` was introduced for JSPI based on a user request to make suspension errors catchable. I could imagine that this is similarly useful for core stack switching? Since handlers are dynamically scoped, applications might not always be able to ensure that there is a handler for the next suspend or switch instruction.
In any case, the current implementation is inconsistent: the builtin still throws `SuspendError` if we don't find a handler. See the `TestSuspendError` test: we throw a `RuntimeError` in the first case, and `SuspendError` in the other cases.
Suggest that we clear this up in a follow-up cl.
kSimdPrefix, 0x11, // i32x4.splatFrancis McCabe```suggestion
kSimdPrefix, kExprI32x4Splat,
```
Acknowledged
I would suggest moving this function to `stack-switching-params.js`. We already test passing SIMD parameters for resume/suspend there, and the test is skipped on no-simd configs (see mjsunit.status) so we don't need this try/catch.
- Darius Mercadier
- Jakob Kummerow
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud voted and added 3 comments![]( "Open in Gerrit")
Votes added by Thibaud Michaud
| Code-Review | +1 |
3 comments
LGTM
IF (is_on_central_stack) {
__ WasmCallRuntime(__ phase_zone(), Runtime::kThrowWasmFXSuspendError, {},Francis McCabeAny reason this isn't a `__ TrapIf(...)` call?
Thibaud MichaudYes, it should be.
The wasmfx explainer is actually silent on this; but, in keeping with other specs, we should trap. I have also adjusted the Suspend implementation.
Francis McCabe`WebAssembly.SuspendError` was introduced for JSPI based on a user request to make suspension errors catchable. I could imagine that this is similarly useful for core stack switching? Since handlers are dynamically scoped, applications might not always be able to ensure that there is a handler for the next suspend or switch instruction.
In any case, the current implementation is inconsistent: the builtin still throws `SuspendError` if we don't find a handler. See the `TestSuspendError` test: we throw a `RuntimeError` in the first case, and `SuspendError` in the other cases.
Suggest that we clear this up in a follow-up cl.
That's fine with me, but right now this change only introduces a new inconsistency. Whether we trap or throw now depends on an implementation detail of the engine (whether we are on the central stack or not).
Can we revert it and make the change globally in the follow-up?
kSimdPrefix, 0xa3, 0x01, // i32x4.all_truenit:
```suggestion
kSimdPrefix, kExprI32AllTrue,
```
Related details
- Darius Mercadier
- Francis McCabe
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis McCabe (Gerrit)
Francis McCabe added 2 comments![]( "Open in Gerrit")
IF (is_on_central_stack) {
__ WasmCallRuntime(__ phase_zone(), Runtime::kThrowWasmFXSuspendError, {},Francis McCabeAny reason this isn't a `__ TrapIf(...)` call?
Thibaud MichaudYes, it should be.
The wasmfx explainer is actually silent on this; but, in keeping with other specs, we should trap. I have also adjusted the Suspend implementation.
Francis McCabe`WebAssembly.SuspendError` was introduced for JSPI based on a user request to make suspension errors catchable. I could imagine that this is similarly useful for core stack switching? Since handlers are dynamically scoped, applications might not always be able to ensure that there is a handler for the next suspend or switch instruction.
In any case, the current implementation is inconsistent: the builtin still throws `SuspendError` if we don't find a handler. See the `TestSuspendError` test: we throw a `RuntimeError` in the first case, and `SuspendError` in the other cases.
Thibaud MichaudSuggest that we clear this up in a follow-up cl.
That's fine with me, but right now this change only introduces a new inconsistency. Whether we trap or throw now depends on an implementation detail of the engine (whether we are on the central stack or not).
Can we revert it and make the change globally in the follow-up?
Acknowledged
kSimdPrefix, 0xa3, 0x01, // i32x4.all_trueFrancis McCabenit:
```suggestion
kSimdPrefix, kExprI32AllTrue,
```
For some reason, that doesnt work. However, I have refactored to:
...SimdInstr(kExprI32x4AllTrue)
which does work.
Related details
- Darius Mercadier
- Jakob Kummerow
- Thibaud Michaud
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 2 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
2 comments
memset(stack->jmpbuf(), 0, sizeof(wasm::JumpBuffer));
stack->jmpbuf()->state = wasm::JumpBuffer::Active;
stack->jmpbuf()->stack_limit = stack->jslimit();
stack->jmpbuf()->parent = nullptr;
stack->jmpbuf()->is_on_central_stack = true;Francis McCabeWhy is this needed?
The jump buffer of the active stack is usually considered to be garbage since the SP, FP, etc. are currently changing. The jump buffer is initialized when we switch to a different stack.
Same thing for the central stack pointers below. They should only be needed after having switched stacks, at which point they should have already been initialized.
In my defense, this was suggested by Gemini.
However, if I dont zero out the jmpbuf, v8 does not build.
For the record, that's not a defense and not an excuse. As the author of a CL, you're responsible for its contents. Don't upload AI hallucinations you don't understand.
kSimdPrefix, 0xa3, 0x01, // i32x4.all_trueFrancis McCabenit:
```suggestion
kSimdPrefix, kExprI32AllTrue,
```
For some reason, that doesnt work. However, I have refactored to:
...SimdInstr(kExprI32x4AllTrue)
which does work.
That "some reason" is that `0xa3, 0x01` is a two-byte LEB sequence, but `kExprI32x4AllTrue` can only emit one number. `...SimdInstr(kExprI32x4AllTrue)` is the way to go, it exists specifically for SIMD instructions that need more than one byte after the `kSimdPrefix`.
Related details
- Darius Mercadier
- Francis McCabe
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
memset(stack->jmpbuf(), 0, sizeof(wasm::JumpBuffer));
stack->jmpbuf()->state = wasm::JumpBuffer::Active;
stack->jmpbuf()->stack_limit = stack->jslimit();
stack->jmpbuf()->parent = nullptr;
stack->jmpbuf()->is_on_central_stack = true;Francis McCabeWhy is this needed?
The jump buffer of the active stack is usually considered to be garbage since the SP, FP, etc. are currently changing. The jump buffer is initialized when we switch to a different stack.
Same thing for the central stack pointers below. They should only be needed after having switched stacks, at which point they should have already been initialized.
Jakob KummerowIn my defense, this was suggested by Gemini.
However, if I dont zero out the jmpbuf, v8 does not build.
For the record, that's not a defense and not an excuse. As the author of a CL, you're responsible for its contents. Don't upload AI hallucinations you don't understand.
+1 to what Jakob said.
And actually I still have issues with this part of the change. You removed this line:
```
stack->jmpbuf()->state = wasm::JumpBuffer::Active;
```
Which was important among other things because we rely on this state to know how to iterate the stack (see `StackMemory::Iterate()`). And in particular, if the stack is active, we know that we should not rely on the jump buffer to find the first frame. Instead, we look at the `ThreadLocalTop*` structure, as we did before introducing stack switching.
`Active` is 0, so by doing `memset(..., 0, ...)` you still happen to set the state correctly, but in a much more obfuscated way. And setting the other values should not be needed: if we read the other values somewhere before initializing the jump buffer, something wrong is going on and we should figure out what and fix it.
Related details
- Darius Mercadier
- Francis McCabe
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis McCabe (Gerrit)
Francis McCabe voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Darius Mercadier
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![]( "Open in Gerrit")
Change information
[wasm][wasmfx] Implement switch instruction.
BYPASS_LARGE_CHANGE_WARNING: Implementing switch instruction on multiple
platforms.
- M src/builtins/arm/builtins-arm.cc
- M src/builtins/arm64/builtins-arm64.cc
- M src/builtins/base.tq
- M src/builtins/builtins-definitions.h
- M src/builtins/ia32/builtins-ia32.cc
- M src/builtins/wasm.tq
- M src/builtins/x64/builtins-x64.cc
- M src/codegen/external-reference.cc
- M src/codegen/external-reference.h
- M src/codegen/interface-descriptors-inl.h
- M src/codegen/interface-descriptors.h
- M src/common/globals.h
- M src/common/message-template.h
- M src/compiler/turboshaft/builtin-call-descriptors.h
- M src/execution/isolate.cc
- M src/wasm/function-body-decoder-impl.h
- M src/wasm/turboshaft-graph-interface.cc
- M src/wasm/wasm-builtin-list.h
- M src/wasm/wasm-external-refs.cc
- M src/wasm/wasm-external-refs.h
- M test/mjsunit/wasm/stack-switching-params.js
- A test/mjsunit/wasm/stack-switching-switch-loop.js
- A test/mjsunit/wasm/stack-switching-switch.js
- M test/wasm-spec-tests/wasm-spec-tests.status
Code-Review: +1 by Jakob Kummerow
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |