[maglev] Migrate ArrayIsArray and Array.at [v8/v8 : main]
0 views
Skip to first unread message
Victor Gomes (Gerrit)
Jun 9, 2026, 10:11:21 AM (2 days ago) Jun 9
to Marja Hölttä, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Marja Hölttä
Victor Gomes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Victor Gomes
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Marja Hölttä
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Jun 9, 2026, 2:23:12 PM (2 days ago) Jun 9
to v8-s...@luci-project-accounts.iam.gserviceaccount.com, Marja Hölttä, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Marja Hölttä
Victor Gomes voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Marja Hölttä
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Jun 10, 2026, 2:12:54 AM (yesterday) Jun 10
to Victor Gomes, v8-s...@luci-project-accounts.iam.gserviceaccount.com, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes
Marja Hölttä voted and added 4 comments![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Code-Review | +1 |
4 comments
Patchset-level comments
File-level comment, Patchset 3 (Latest):
Marja Hölttä . resolved
LGTM but....
I caught these by doing the "AI review" thing; I no longer trust the "mechanical" changes by AI :/ I wonder what's in the previous mechanical changes which I also didn't review line-by-line
File src/maglev/maglev-graph-builder.cc
Line 4202, Patchset 3 (Parent):
// We won't try to reason about the type of the elements array and thus also
// cannot end up with an empty type for it.
DCHECK(!IsEmptyNodeType(GetType(elements)));Marja Hölttä . unresolved
this is now gone, is that intentional?
Line 4214, Patchset 3 (Parent):
// Initializing stores can place conversion nodes (e.g.
// ChangeInt32ToFloat64) into the virtual object, but conversion nodes
// must not leak into the interpreter frame state -- they belong in
// NodeInfo as alternative representations.Marja Hölttä . unresolved
now this comment is gone, is that okay?
Line 8730, Patchset 3 (Parent):
// TODO(42204525): Support polymorphism. I.e., DOUBLE_ELEMENTS and ELEMENTS
// together.Marja Hölttä . unresolved
this comment is now gone too
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)
Jun 10, 2026, 3:12:36 AM (yesterday) Jun 10
to Marja Hölttä, v8-s...@luci-project-accounts.iam.gserviceaccount.com, dmercadi...@chromium.org, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Victor Gomes voted and added 4 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Auto-Submit | +1 |
| Commit-Queue | +2 |
4 comments
Patchset-level comments
File src/maglev/maglev-graph-builder.cc
Line 4202, Patchset 3 (Parent):
// We won't try to reason about the type of the elements array and thus also
// cannot end up with an empty type for it.
DCHECK(!IsEmptyNodeType(GetType(elements)));Marja Hölttä . resolved
this is now gone, is that intentional?
Victor Gomes
Done
Line 4214, Patchset 3 (Parent):
// Initializing stores can place conversion nodes (e.g.
// ChangeInt32ToFloat64) into the virtual object, but conversion nodes
// must not leak into the interpreter frame state -- they belong in
// NodeInfo as alternative representations.Marja Hölttä . resolved
now this comment is gone, is that okay?
Victor Gomes
Done
Line 8730, Patchset 3 (Parent):
// TODO(42204525): Support polymorphism. I.e., DOUBLE_ELEMENTS and ELEMENTS
// together.Marja Hölttä . resolved
this comment is now gone too
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)
Jun 10, 2026, 3:53:53 AM (yesterday) Jun 10
to Victor Gomes, Marja Hölttä, 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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/maglev/maglev-reducer-inl.h
Insertions: 5, Deletions: 0.
@@ -512,6 +512,9 @@
template <typename BaseT>
ReduceResult MaglevReducer<BaseT>::BuildLoadFixedDoubleArrayElement(
ValueNode* elements, ValueNode* index) {
+ // We won't try to reason about the type of the elements array and thus also
+ // cannot end up with an empty type for it.
+ DCHECK(!IsEmptyNodeType(GetType(elements)));
if constexpr (ReducerBaseWithAllocationTracking<BaseT>) {
if (auto constant = TryGetInt32Constant(index)) {
RETURN_IF_DONE(base_->TryBuildLoadFixedDoubleArrayElementFromAllocation(
@@ -985,6 +988,8 @@
MapInference<MaglevReducer<BaseT>> inference(this, receiver);
auto possible_maps = inference.TryGetPossibleMaps();
ElementsKind elements_kind = NO_ELEMENTS;
+ // TODO(42204525): Support polymorphism. I.e., DOUBLE_ELEMENTS and ELEMENTS
+ // together.
if (!possible_maps || !CanInlineArrayIteratingBuiltin(
broker(), *possible_maps, &elements_kind)) {
return {};
```
```
The name of the file: src/maglev/maglev-graph-builder.cc
Insertions: 4, Deletions: 0.
@@ -4143,6 +4143,10 @@
if (static_cast<uint32_t>(index) >= length.value()) {
return BuildAbort(AbortReason::kUnreachable);
}
+ // Initializing stores can place conversion nodes (e.g.
+ // ChangeInt32ToFloat64) into the virtual object, but conversion nodes
+ // must not leak into the interpreter frame state -- they belong in
+ // NodeInfo as alternative representations.
return vobject->get(FixedDoubleArray::OffsetOfElementAt(index))->Unwrap();
}
```
Change information
Commit message:
[maglev] Migrate ArrayIsArray and Array.at
Bug: 424157317
Change-Id: If2972c351d7a831104f86099282f5a6b3c713c6c
Reviewed-by: Marja Hölttä <ma...@chromium.org>
Commit-Queue: Victor Gomes <victo...@chromium.org>
Auto-Submit: Victor Gomes <victo...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#107874}
Files:
- M src/maglev/maglev-graph-builder.cc
- M src/maglev/maglev-graph-builder.h
- M src/maglev/maglev-reducer-inl.h
- M src/maglev/maglev-reducer.h
- A test/mjsunit/turbolev/optimizer-reduce-array-at.js
- A test/mjsunit/turbolev/optimizer-reduce-array-isarray.js
Change size: L
Delta: 6 files changed, 310 insertions(+), 195 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Marja Hölttä
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages