[compiler][turboshaft] Decouple eager and lazy frame states [v8/v8 : main]
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Auto-Submit | +1 |
1 comment
Related details
- Daniel Lehmann
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 4 comments![]( "Open in Gerrit")
Nico, PTAL overall; Daniel PTAL Wasm.
I reviewed the Wasm parts but have some high-level questions to fill my knowledge gap/uncertainty about eager vs. lazy frame states. Also CC'ing Matthias and Paolo, just FYI since we dealt with a bunch of issues in the wrapper/Wasm inlining in the past.
return __ template FrameState<EagerFrameState>(This might be a stupid question or I might simply be missing background knowledge about eager vs. lazy frame states:
Why is this frame state for the inlined Wasm body an `EagerFrameState`, but the one for the continuation a `LazyFrameState` one? Is it because the first is used when we throw an exception in the inlined Wasm body vs. the other is when a transitively called JS function causes a lazy deopt of the caller into which the Wasm body was inlined?
My understanding still seems to be a bit hazy, sometimes we chose eager vs. lazy based on the operation (e.g., `Call` always has a `LazyFrameState` attached?), sometimes based on whether the frame state describes the state of the execution before the operation vs. after, and sometimes based on the type of deopt that consumes the frame state, and theoretically all these should come to the same conclusion, but I'm not sure.
V<LazyFrameState> frame_state) {What I don't understand is: here, where we basically strip/unwrap the `ProcessWasmArgument` ops, this `frame_state` is declared as a `LazyFrameState`, but below in `REDUCE_INPUT_GRAPH(Call)`, we stash it in `pending_caller_frame_state_` as an `EagerFrameState`. Which is correct?
From my understanding, we use the `pending_caller_frame_state_` in the eager deopts during JS-to-Wasm argument conversion, i.e., it should be an eager frame state, right?
Also, given that this change intends to make accidental misused of eager vs. lazy frame states harder, how come there is no cast here, even though the type apparently changes?
// TODO(dmercadier, dlehmann): Since this frame state is only used for stackMakes sense, but if we do that, we'd have to "make sure" we don't use it for anything that needs locals etc. in the future. At the very least by renaming this function to something like `CreateFrameStateForStackTrace` or something, or maybe something more robust, some `DCHECK`?
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
2 comments
return __ template FrameState<EagerFrameState>(This might be a stupid question or I might simply be missing background knowledge about eager vs. lazy frame states:
Why is this frame state for the inlined Wasm body an `EagerFrameState`, but the one for the continuation a `LazyFrameState` one? Is it because the first is used when we throw an exception in the inlined Wasm body vs. the other is when a transitively called JS function causes a lazy deopt of the caller into which the Wasm body was inlined?
My understanding still seems to be a bit hazy, sometimes we chose eager vs. lazy based on the operation (e.g., `Call` always has a `LazyFrameState` attached?), sometimes based on whether the frame state describes the state of the execution before the operation vs. after, and sometimes based on the type of deopt that consumes the frame state, and theoretically all these should come to the same conclusion, but I'm not sure.
I mean this is the same as below where eager vs. lazy doesn't really mean anything as it's for error locations only?
It would only matter if we started performing deopts (e.g. for a "deopt on throw" for any of these traps)
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 1 comment![]( "Open in Gerrit")
return __ template FrameState<EagerFrameState>(Matthias LiedtkeThis might be a stupid question or I might simply be missing background knowledge about eager vs. lazy frame states:
Why is this frame state for the inlined Wasm body an `EagerFrameState`, but the one for the continuation a `LazyFrameState` one? Is it because the first is used when we throw an exception in the inlined Wasm body vs. the other is when a transitively called JS function causes a lazy deopt of the caller into which the Wasm body was inlined?
My understanding still seems to be a bit hazy, sometimes we chose eager vs. lazy based on the operation (e.g., `Call` always has a `LazyFrameState` attached?), sometimes based on whether the frame state describes the state of the execution before the operation vs. after, and sometimes based on the type of deopt that consumes the frame state, and theoretically all these should come to the same conclusion, but I'm not sure.
I mean this is the same as below where eager vs. lazy doesn't really mean anything as it's for error locations only?
It would only matter if we started performing deopts (e.g. for a "deopt on throw" for any of these traps)
Ah, good point: the frame state for the inlined Wasm body is used for trapping Wasm ops, and in that case we are not continuing execution after the trap anyway, so eager vs. lazy simply doesn't matter. Maybe we should add that as a comment.
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 3 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Auto-Submit | +1 |
3 comments
Matthias LiedtkeThis might be a stupid question or I might simply be missing background knowledge about eager vs. lazy frame states:
Why is this frame state for the inlined Wasm body an `EagerFrameState`, but the one for the continuation a `LazyFrameState` one? Is it because the first is used when we throw an exception in the inlined Wasm body vs. the other is when a transitively called JS function causes a lazy deopt of the caller into which the Wasm body was inlined?
My understanding still seems to be a bit hazy, sometimes we chose eager vs. lazy based on the operation (e.g., `Call` always has a `LazyFrameState` attached?), sometimes based on whether the frame state describes the state of the execution before the operation vs. after, and sometimes based on the type of deopt that consumes the frame state, and theoretically all these should come to the same conclusion, but I'm not sure.
Daniel LehmannI mean this is the same as below where eager vs. lazy doesn't really mean anything as it's for error locations only?
It would only matter if we started performing deopts (e.g. for a "deopt on throw" for any of these traps)
Ah, good point: the frame state for the inlined Wasm body is used for trapping Wasm ops, and in that case we are not continuing execution after the trap anyway, so eager vs. lazy simply doesn't matter. Maybe we should add that as a comment.
I've added a comment. This is again a weird Wasm case where we don't perform a real deopt but just need a FrameState for error location.
What I don't understand is: here, where we basically strip/unwrap the `ProcessWasmArgument` ops, this `frame_state` is declared as a `LazyFrameState`, but below in `REDUCE_INPUT_GRAPH(Call)`, we stash it in `pending_caller_frame_state_` as an `EagerFrameState`. Which is correct?
From my understanding, we use the `pending_caller_frame_state_` in the eager deopts during JS-to-Wasm argument conversion, i.e., it should be an eager frame state, right?Also, given that this change intends to make accidental misused of eager vs. lazy frame states harder, how come there is no cast here, even though the type apparently changes?
Whups, good catch.
REDUCE methods are called in a generic way that passes raw OpIndex as arguments, which are convertible to both LazyFrameState and EagerFrameState... maybe we could add some verification to REDUCE that the list of arguments actually matches what the constructor of the operation expects; I'll think about it, but that will be for a follow-up.
(and yea, this should be an Eager frame state, not a Lazy one)
// TODO(dmercadier, dlehmann): Since this frame state is only used for stackMakes sense, but if we do that, we'd have to "make sure" we don't use it for anything that needs locals etc. in the future. At the very least by renaming this function to something like `CreateFrameStateForStackTrace` or something, or maybe something more robust, some `DCHECK`?
DCHECKing anything useful here is hard, but I've at least renamed the function.
Related details
- Daniel Lehmann
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Lehmann
| Code-Review | +1 |
2 comments
Thanks for the additional background!
return __ template FrameState<EagerFrameState>(Matthias LiedtkeThis might be a stupid question or I might simply be missing background knowledge about eager vs. lazy frame states:
Why is this frame state for the inlined Wasm body an `EagerFrameState`, but the one for the continuation a `LazyFrameState` one? Is it because the first is used when we throw an exception in the inlined Wasm body vs. the other is when a transitively called JS function causes a lazy deopt of the caller into which the Wasm body was inlined?
My understanding still seems to be a bit hazy, sometimes we chose eager vs. lazy based on the operation (e.g., `Call` always has a `LazyFrameState` attached?), sometimes based on whether the frame state describes the state of the execution before the operation vs. after, and sometimes based on the type of deopt that consumes the frame state, and theoretically all these should come to the same conclusion, but I'm not sure.
Daniel LehmannI mean this is the same as below where eager vs. lazy doesn't really mean anything as it's for error locations only?
It would only matter if we started performing deopts (e.g. for a "deopt on throw" for any of these traps)
Darius MercadierAh, good point: the frame state for the inlined Wasm body is used for trapping Wasm ops, and in that case we are not continuing execution after the trap anyway, so eager vs. lazy simply doesn't matter. Maybe we should add that as a comment.
I've added a comment. This is again a weird Wasm case where we don't perform a real deopt but just need a FrameState for error location.
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Hartmann (Gerrit)
Nico Hartmann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nico Hartmann
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
LGTM
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-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[turboshaft] Decouple eager and lazy frame states
This CL decouples turboshaft::FrameState into three distinct types:
EagerFrameState, LazyFrameState, and AnyFrameState (an untagged
union).
Previously, a single generic FrameState type was used all over
the compiler. This lacked type safety and made it easy to accidentally
pass lazy frame states to eager deopt instructions or vice versa.
Detailed changes:
- Introduced EagerFrameState, LazyFrameState, and AnyFrameState in
index.h.
- Enforced strict, non-implicit compile-time typing by ensuring that
specific frame states (EagerFrameState / LazyFrameState) are never
implicitly constructible from the generic AnyFrameState. Any
retargeting or conversion between eager and lazy deopt points must
be explicitly cast in the source code.
- Added compile-time static assertions inside OperationT::input<T>()
to permanently prevent AnyFrameState from being used as an input type
in all Turboshaft Operation classes (except FrameStateOp which uses
an explicit manual cast bypass for parent frame states).
- Refactored all Reducers to enforce precise eager/lazy frame states.
NO_IFTTT=only updates V<> types, which don't exist in Maglev; no
functional change.
TAG=agy
CONV=6bf1b779-ee18-485a-b5fd-ff1cccf6330c
- M src/codegen/turboshaft-builtins-assembler-inl.h
- M src/compiler/backend/instruction-selector.h
- M src/compiler/turboshaft/assembler.h
- M src/compiler/turboshaft/branch-elimination-reducer.h
- M src/compiler/turboshaft/copying-phase.h
- M src/compiler/turboshaft/deopt-data.h
- M src/compiler/turboshaft/fast-api-call-lowering-reducer.h
- M src/compiler/turboshaft/graph-builder.cc
- M src/compiler/turboshaft/index.h
- M src/compiler/turboshaft/int64-lowering-reducer.h
- M src/compiler/turboshaft/js-generic-lowering-reducer.h
- M src/compiler/turboshaft/machine-lowering-reducer-inl.h
- M src/compiler/turboshaft/machine-optimization-reducer.h
- M src/compiler/turboshaft/operations.h
- M src/compiler/turboshaft/runtime-call-descriptors.h
- M src/compiler/turboshaft/simplified-optimization-reducer.h
- M src/compiler/turboshaft/stack-check-lowering-reducer.h
- M src/compiler/turboshaft/string-escape-analysis-reducer.cc
- M src/compiler/turboshaft/string-escape-analysis-reducer.h
- M src/compiler/turboshaft/turbolev-early-lowering-reducer-inl.h
- M src/compiler/turboshaft/turbolev-graph-builder.cc
- M src/compiler/turboshaft/variable-reducer.h
- M src/compiler/turboshaft/wasm-in-js-inlining-reducer-inl.h
- M src/compiler/turboshaft/wasm-js-lowering-reducer.h
- M src/compiler/turboshaft/wasm-lowering-reducer.h
- M src/compiler/turboshaft/wasm-wrappers-inl.h
- M src/compiler/turboshaft/wasm-wrappers.h
- M src/wasm/turboshaft-graph-interface.cc
- M src/wasm/turboshaft-graph-interface.h
- M test/unittests/compiler/turboshaft/reducer-test.h
Code-Review: +1 by Nico Hartmann, +1 by Matthias Liedtke, +1 by Daniel Lehmann
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |