[maglev] Refactor context aliasing logic [v8/v8 : main]

0 views
Skip to first unread message

Victor Gomes (Gerrit)

unread,
4:35 AM (7 hours ago) 4:35 AM
to Marja Hölttä, 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

Votes added by Victor Gomes

Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Victor Gomes . resolved

PTAL!

Open in Gerrit

Related details

Attention is currently required from:
  • Marja Hölttä
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: I7158fb12c2295881c38efc6f5ee42c528ca1b1c8
Gerrit-Change-Number: 7077318
Gerrit-PatchSet: 1
Gerrit-Owner: Victor Gomes <victo...@chromium.org>
Gerrit-Reviewer: Marja Hölttä <ma...@chromium.org>
Gerrit-Reviewer: Victor Gomes <victo...@chromium.org>
Gerrit-Attention: Marja Hölttä <ma...@chromium.org>
Gerrit-Comment-Date: Thu, 23 Oct 2025 08:35:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Marja Hölttä (Gerrit)

unread,
4:46 AM (7 hours ago) 4:46 AM
to Victor Gomes, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes

Marja Hölttä voted and added 3 comments

Votes added by Marja Hölttä

Code-Review+1

3 comments

Patchset-level comments
Marja Hölttä . resolved

lgtm %

File src/maglev/maglev-graph.cc
Line 112, Patchset 1 (Latest): if ((true) || !v8_flags.reuse_scope_infos) return true;
Marja Hölttä . unresolved

Just to check my understanding: It's intentional that this func now always returns true, an in the future it will always return true if the --reuse-scope-infos is not enabled?

File src/maglev/maglev-known-node-aspects.cc
Line 472, Patchset 1 (Latest): compiler::OptionalScopeInfoRef scope_info =
graph->TryGetScopeInfo(context);
Marja Hölttä . unresolved

This could be moved out of the loop?

Open in Gerrit

Related details

Attention is currently required from:
  • Victor Gomes
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: I7158fb12c2295881c38efc6f5ee42c528ca1b1c8
Gerrit-Change-Number: 7077318
Gerrit-PatchSet: 1
Gerrit-Owner: Victor Gomes <victo...@chromium.org>
Gerrit-Reviewer: Marja Hölttä <ma...@chromium.org>
Gerrit-Reviewer: Victor Gomes <victo...@chromium.org>
Gerrit-Attention: Victor Gomes <victo...@chromium.org>
Gerrit-Comment-Date: Thu, 23 Oct 2025 08:46:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

chromeperf@appspot.gserviceaccount.com (Gerrit)

unread,
4:46 AM (7 hours ago) 4:46 AM
to Victor Gomes, Marja Hölttä, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes

Message from chrom...@appspot.gserviceaccount.com

😿 Job mac-m1_mini_2020-perf/jetstream2 failed.

See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17030785510000

Open in Gerrit

Related details

Attention is currently required from:
  • Victor Gomes
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: I7158fb12c2295881c38efc6f5ee42c528ca1b1c8
Gerrit-Change-Number: 7077318
Gerrit-PatchSet: 1
Gerrit-Owner: Victor Gomes <victo...@chromium.org>
Gerrit-Reviewer: Marja Hölttä <ma...@chromium.org>
Gerrit-Reviewer: Victor Gomes <victo...@chromium.org>
Gerrit-Attention: Victor Gomes <victo...@chromium.org>
Gerrit-Comment-Date: Thu, 23 Oct 2025 08:46:50 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Victor Gomes (Gerrit)

unread,
4:49 AM (7 hours ago) 4:49 AM
to chrom...@appspot.gserviceaccount.com, Marja Hölttä, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

Victor Gomes voted and added 3 comments

Votes added by Victor Gomes

Commit-Queue+2

3 comments

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Victor Gomes . resolved

Thanks!

File src/maglev/maglev-graph.cc
Line 112, Patchset 1: if ((true) || !v8_flags.reuse_scope_infos) return true;
Marja Hölttä . resolved

Just to check my understanding: It's intentional that this func now always returns true, an in the future it will always return true if the --reuse-scope-infos is not enabled?

Victor Gomes

Yeah, I just copied and pasted this function. There is a TODO there, I didn't want to change. But you're right, the logic is disabled for now. We should try to re-enable it in the future.

File src/maglev/maglev-known-node-aspects.cc
Line 472, Patchset 1: compiler::OptionalScopeInfoRef scope_info =
graph->TryGetScopeInfo(context);
Marja Hölttä . resolved

This could be moved out of the loop?

Victor Gomes

Indeed. Thanks!

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
    • requirement satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: I7158fb12c2295881c38efc6f5ee42c528ca1b1c8
    Gerrit-Change-Number: 7077318
    Gerrit-PatchSet: 2
    Gerrit-Owner: Victor Gomes <victo...@chromium.org>
    Gerrit-Reviewer: Marja Hölttä <ma...@chromium.org>
    Gerrit-Reviewer: Victor Gomes <victo...@chromium.org>
    Gerrit-Comment-Date: Thu, 23 Oct 2025 08:49:41 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Marja Hölttä <ma...@chromium.org>
    satisfied_requirement
    open
    diffy

    V8 LUCI CQ (Gerrit)

    unread,
    5:30 AM (6 hours ago) 5:30 AM
    to Victor Gomes, chrom...@appspot.gserviceaccount.com, Marja Hölttä, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

    V8 LUCI CQ submitted the change with unreviewed changes

    Unreviewed changes

    1 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-known-node-aspects.cc
    Insertions: 1, Deletions: 2.

    @@ -468,9 +468,8 @@
    graph->broker()->local_isolate(), context);
    }
    if (may_have_aliasing_contexts() == ContextSlotLoadsAlias::kYes) {
    + compiler::OptionalScopeInfoRef scope_info = graph->TryGetScopeInfo(context);
    for (auto& cache : loaded_context_slots_) {
    - compiler::OptionalScopeInfoRef scope_info =
    - graph->TryGetScopeInfo(context);
    int cached_offset = std::get<int>(cache.first);
    ValueNode* cached_context = std::get<ValueNode*>(cache.first);
    if (cached_offset == offset && cached_context != context) {
    ```

    Change information

    Commit message:
    [maglev] Refactor context aliasing logic

    It moves the context aliasing logic to the KNA, so
    that it can be used by other compiler passes.
    Bug: 424157317
    Change-Id: I7158fb12c2295881c38efc6f5ee42c528ca1b1c8
    Commit-Queue: Victor Gomes <victo...@chromium.org>
    Reviewed-by: Marja Hölttä <ma...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#103306}
    Files:
    • M src/maglev/maglev-graph-builder.cc
    • M src/maglev/maglev-graph.cc
    • M src/maglev/maglev-graph.h
    • M src/maglev/maglev-known-node-aspects.cc
    • M src/maglev/maglev-known-node-aspects.h
    Change size: M
    Delta: 5 files changed, 84 insertions(+), 40 deletions(-)
    Branch: refs/heads/main
    Submit Requirements:
    • requirement satisfiedCode-Review: +1 by Marja Hölttä
    Open in Gerrit
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: merged
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: I7158fb12c2295881c38efc6f5ee42c528ca1b1c8
    Gerrit-Change-Number: 7077318
    Gerrit-PatchSet: 3
    Gerrit-Owner: Victor Gomes <victo...@chromium.org>
    Gerrit-Reviewer: Marja Hölttä <ma...@chromium.org>
    open
    diffy
    satisfied_requirement
    Reply all
    Reply to author
    Forward
    0 new messages