[maglev] Port FixedDoubleArray to new VirtualObject layout [v8/v8 : main]

[Jakob Linke (Gerrit)]

Jakob Linke added 2 comments

File src/compiler/turboshaft/turbolev-graph-builder.cc

Line 5503, Patchset 1 (Latest): builder.AddInput(

Jakob Linke . unresolved

Again, this is just different enough so we can't trivially fold it into the default path :|

Here: the hole_nan to the_hole_value conversion, which doesn't happen in AddVirtualObjectNestedValue.

File src/maglev/maglev-code-generator.cc

Line 1527, Patchset 1 (Latest): if (value.is_hole_nan()) {

Jakob Linke . unresolved

Here: again the hole conversion, and NewHeapNumberFromBits, whereas BuildNestedValue just does NewNumber (including smi canonicalization and I suppose without preserving the bit pattern).

Maybe we could pull these things into AddVirtualObjectNestedValue/BuildNestedValue and just take the normal paths instead..

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I799eded266793295b02b2d190dd8a1a56159dcdc

Gerrit-Change-Number: 6975071

Gerrit-PatchSet: 1

Gerrit-Owner: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Jakob Linke <jgr...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Tue, 23 Sep 2025 11:07:37 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Jakob Linke (Gerrit)]

Attention needed from Victor Gomes

New activity on the change

Open in Gerrit

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.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I799eded266793295b02b2d190dd8a1a56159dcdc

Gerrit-Change-Number: 6975071

Gerrit-PatchSet: 5

Gerrit-Owner: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Victor Gomes <victo...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Victor Gomes <victo...@chromium.org>

Gerrit-Comment-Date: Fri, 26 Sep 2025 05:57:21 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Victor Gomes (Gerrit)]

Attention needed from Jakob Linke

Victor Gomes voted and added 4 comments

Votes added by Victor Gomes

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Victor Gomes . resolved

LGTM

File src/compiler/turboshaft/turbolev-graph-builder.cc

Line 5503, Patchset 1: builder.AddInput(

Jakob Linke . resolved

Again, this is just different enough so we can't trivially fold it into the default path :|

Here: the hole_nan to the_hole_value conversion, which doesn't happen in AddVirtualObjectNestedValue.

Victor Gomes

Acknowledged

File src/maglev/maglev-code-generator.cc

Line 1527, Patchset 1: if (value.is_hole_nan()) {

Jakob Linke . unresolved

Here: again the hole conversion, and NewHeapNumberFromBits, whereas BuildNestedValue just does NewNumber (including smi canonicalization and I suppose without preserving the bit pattern).

Maybe we could pull these things into AddVirtualObjectNestedValue/BuildNestedValue and just take the normal paths instead..

Victor Gomes

I guess the advantage of using `GetDeoptLiteral(roots.the_hole_value()))` is a smaller `TranslationArray`. We could probably avoid creating the HN here as well... I don't know why we do it. My guess the deoptimizer unwraps the HN. Maybe we could send the raw values instead.

BuildNestedValue probably doesn't care about holes, but I am not sure.

Making the code more consistent in all code paths sgtm.

File src/maglev/maglev-ir.h

Line 6727, Patchset 5 (Latest): const vobj::ObjectLayout* object_layout_ = nullptr;

Victor Gomes . unresolved

Do we ever create a VO without ObjectLayout?
I guess this could also be a `const vobj::ObjectLayout&` to avoid nullptr issues.
It could be a follow up...

Open in Gerrit

Related details

Attention is currently required from:

• Jakob Linke

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.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I799eded266793295b02b2d190dd8a1a56159dcdc

Gerrit-Change-Number: 6975071

Gerrit-PatchSet: 5

Gerrit-Owner: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Victor Gomes <victo...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Jakob Linke <jgr...@chromium.org>

Gerrit-Comment-Date: Fri, 26 Sep 2025 08:26:57 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Jakob Linke <jgr...@chromium.org>

[Jakob Linke (Gerrit)]

Jakob Linke voted and added 3 comments

Votes added by Jakob Linke

Commit-Queue

+2

3 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Jakob Linke . resolved

Thanks for the review!

File src/maglev/maglev-code-generator.cc

Line 1527, Patchset 1: if (value.is_hole_nan()) {

Jakob Linke . resolved

Here: again the hole conversion, and NewHeapNumberFromBits, whereas BuildNestedValue just does NewNumber (including smi canonicalization and I suppose without preserving the bit pattern).

Maybe we could pull these things into AddVirtualObjectNestedValue/BuildNestedValue and just take the normal paths instead..

Victor Gomes

I guess the advantage of using `GetDeoptLiteral(roots.the_hole_value()))` is a smaller `TranslationArray`. We could probably avoid creating the HN here as well... I don't know why we do it. My guess the deoptimizer unwraps the HN. Maybe we could send the raw values instead.

BuildNestedValue probably doesn't care about holes, but I am not sure.

Making the code more consistent in all code paths sgtm.

Jakob Linke

Ack. I will have a closer look once this cl series is complete.

File src/maglev/maglev-ir.h

Line 6727, Patchset 5 (Latest): const vobj::ObjectLayout* object_layout_ = nullptr;

Victor Gomes . resolved

Do we ever create a VO without ObjectLayout?
I guess this could also be a `const vobj::ObjectLayout&` to avoid nullptr issues.
It could be a follow up...

Jakob Linke

It could, yes, but I don't see much difference vs. the current impl with a DCHECK_NOT_NULL. We never dynamically select an ObjectLayout.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I799eded266793295b02b2d190dd8a1a56159dcdc

Gerrit-Change-Number: 6975071

Gerrit-PatchSet: 5

Gerrit-Owner: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Victor Gomes <victo...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Fri, 26 Sep 2025 10:47:46 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Jakob Linke <jgr...@chromium.org>

Comment-In-Reply-To: Victor Gomes <victo...@chromium.org>

[V8 LUCI CQ (Gerrit)]

V8 LUCI CQ submitted the change

Change information

Commit message:

[maglev] Port FixedDoubleArray to new VirtualObject layout

Change-Id: I799eded266793295b02b2d190dd8a1a56159dcdc

Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6975071

Commit-Queue: Jakob Linke <jgr...@chromium.org>

Reviewed-by: Victor Gomes <victo...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#102788}

Files:

• M src/compiler/turboshaft/turbolev-graph-builder.cc
• M src/maglev/maglev-code-generator.cc
• M src/maglev/maglev-graph-builder.cc
• M src/maglev/maglev-graph-builder.h
• M src/maglev/maglev-ir.h

Change size: L

Delta: 5 files changed, 153 insertions(+), 172 deletions(-)

Branch: refs/heads/main

Submit Requirements:

• Code-Review: +1 by Victor Gomes

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I799eded266793295b02b2d190dd8a1a56159dcdc

Gerrit-Change-Number: 6975071

Gerrit-PatchSet: 6

Gerrit-Owner: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: Jakob Linke <jgr...@chromium.org>

Gerrit-Reviewer: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>