[turbolev] Share subgraph construction between builder and optimizer [v8/v8 : main]
0 views
Skip to first unread message
Leszek Swirski (Gerrit)
May 22, 2026, 3:42:57 AM (9 days ago) May 22
to Victor Gomes, Hannes Payer, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, hongcha...@chromium.org, oilpan-r...@chromium.org, rtoy+...@chromium.org, devtools-...@chromium.org, was...@google.com, mlippau...@chromium.org, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes
Leszek Swirski voted and added 2 comments![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
2 comments
Patchset-level comments
File-level comment, Patchset 24 (Latest):
Leszek Swirski . resolved
sorry for the slow review, this looks really good.
File src/maglev/maglev-graph-optimizer.cc
Line 127, Patchset 24 (Latest):
// We need to set a context, since this is unconditioned in the frame state.
// We can set it to undefined, since this will never be executed.
variable_frame_.set(interpreter::Register::current_context(),
reducer_->GetRootConstant(RootIndex::kUndefinedValue));Leszek Swirski . unresolved
can we make this more robust somehow, enforce that it's never used?
Related details
Attention is currently required from:
- Victor Gomes
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
May 22, 2026, 9:07:46 AM (9 days ago) May 22
to Leszek Swirski, Hannes Payer, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, hongcha...@chromium.org, oilpan-r...@chromium.org, rtoy+...@chromium.org, devtools-...@chromium.org, was...@google.com, mlippau...@chromium.org, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Leszek Swirski
Victor Gomes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
Patchset-level comments
File src/maglev/maglev-graph-optimizer.cc
Line 127, Patchset 24:
// We need to set a context, since this is unconditioned in the frame state.
// We can set it to undefined, since this will never be executed.
variable_frame_.set(interpreter::Register::current_context(),
reducer_->GetRootConstant(RootIndex::kUndefinedValue));Leszek Swirski . resolved
can we make this more robust somehow, enforce that it's never used?
Victor Gomes
Right. A nullptr didn't work here, because we walk the frame in the visitor and crash there. But we can create a DeadValue. If we try to use/emit this value, we would crash in UNREACHABLE, wdyt?
Related details
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
May 22, 2026, 9:10:19 AM (9 days ago) May 22
to Victor Gomes, Hannes Payer, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, hongcha...@chromium.org, oilpan-r...@chromium.org, rtoy+...@chromium.org, devtools-...@chromium.org, was...@google.com, mlippau...@chromium.org, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
File src/maglev/maglev-graph-optimizer.cc
Line 127, Patchset 24:
// We need to set a context, since this is unconditioned in the frame state.
// We can set it to undefined, since this will never be executed.
variable_frame_.set(interpreter::Register::current_context(),
reducer_->GetRootConstant(RootIndex::kUndefinedValue));Leszek Swirski . resolved
Victor Gomescan we make this more robust somehow, enforce that it's never used?
Right. A nullptr didn't work here, because we walk the frame in the visitor and crash there. But we can create a DeadValue. If we try to use/emit this value, we would crash in UNREACHABLE, wdyt?
Attention is currently required from:
- Victor Gomes
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
May 22, 2026, 9:10:21 AM (9 days ago) May 22
to Victor Gomes, Hannes Payer, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, hongcha...@chromium.org, oilpan-r...@chromium.org, rtoy+...@chromium.org, devtools-...@chromium.org, was...@google.com, mlippau...@chromium.org, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes
Leszek Swirski voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Victor Gomes
Submit Requirements:
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)
May 22, 2026, 9:41:37 AM (9 days ago) May 22
to Victor Gomes, Leszek Swirski, Hannes Payer, android-bu...@system.gserviceaccount.com, hongcha...@chromium.org, oilpan-r...@chromium.org, rtoy+...@chromium.org, devtools-...@chromium.org, was...@google.com, mlippau...@chromium.org, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[turbolev] Share subgraph construction between builder and optimizer
Introduce Subgraph<BaseT> on MaglevReducer with two specializations:
- Subgraph<MaglevGraphBuilder> facades the existing MaglevSubGraphBuilder.
- Subgraph<MaglevGraphOptimizer> synthesises an off-graph CFG and records a pending splice The GraphProcessor stitches the new blocks into the live graph.
MaglevReducer::Select is a single generic impl over Subgraph<BaseT>
primitives. TryReduceMathMin/Max move from MaglevGraphBuilder to
MaglevReducer so both bases share the reduction; the optimizer side now
reduces Math.min/Math.max for the post-inline CallKnownBuiltin path.
Bug: 424157317
Change-Id: Id172df0079c50995ac4d67aa1a124440575a58b1
Commit-Queue: Victor Gomes <victo...@chromium.org>
Auto-Submit: Victor Gomes <victo...@chromium.org>
Commit-Queue: Leszek Swirski <les...@chromium.org>
Reviewed-by: Leszek Swirski <les...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#107528}
Files:
- M src/compiler/turboshaft/turbolev-graph-builder.cc
- M src/maglev/maglev-graph-builder.cc
- M src/maglev/maglev-graph-builder.h
- M src/maglev/maglev-graph-optimizer.cc
- M src/maglev/maglev-graph-optimizer.h
- M src/maglev/maglev-graph-processor.h
- M src/maglev/maglev-graph.cc
- M src/maglev/maglev-graph.h
- M src/maglev/maglev-interpreter-frame-state.cc
- M src/maglev/maglev-interpreter-frame-state.h
- M src/maglev/maglev-ir.h
- M src/maglev/maglev-kna-processor.h
- M src/maglev/maglev-reducer-inl.h
- M src/maglev/maglev-reducer.h
- A test/mjsunit/turbolev/optimizer-reduce-math-max.js
Change size: XL
Delta: 15 files changed, 1173 insertions(+), 453 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Leszek Swirski
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages