Victor Gomes (Gerrit)

unread,

Sep 26, 2025, 5:14:08 AM (yesterday) Sep 26

to Darius Mercadier, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

Attention needed from Darius Mercadier

Victor Gomes voted and added 1 comment

Votes added by Victor Gomes

Auto-Submit	+1
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:

Darius Mercadier

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darius Mercadier (Gerrit)

unread,

Sep 26, 2025, 5:15:03 AM (yesterday) Sep 26

to Victor Gomes, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

Attention needed from Victor Gomes

Darius Mercadier voted and added 2 comments

Votes added by Darius Mercadier

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Darius Mercadier . resolved

LGTM % comment :)

File src/maglev/maglev-kna-processor.h

Line 63, Patchset 1 (Latest):

      BasicBlock* next_block = block;
      while (next_block->is_edge_split_block()) {
        next_block = next_block->control_node()->Cast<Jump>()->target();
      }
      known_node_aspects_ = next_block->state()->CloneKnownNodeAspects(zone());

Darius Mercadier . unresolved

```suggestion
      while (block->is_edge_split_block()) {
        block = block->control_node()->Cast<Jump>()->target();
      }
      known_node_aspects_ = block->state()->CloneKnownNodeAspects(zone());
```

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

satisfied_requirement

unsatisfied_requirement

open

diffy

Victor Gomes (Gerrit)

unread,

Sep 26, 2025, 5:16:39 AM (yesterday) Sep 26

to Darius Mercadier, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

Victor Gomes added 2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Victor Gomes . resolved

Thanks!

File src/maglev/maglev-kna-processor.h

Line 63, Patchset 1 (Latest):

      BasicBlock* next_block = block;
      while (next_block->is_edge_split_block()) {
        next_block = next_block->control_node()->Cast<Jump>()->target();
      }
      known_node_aspects_ = next_block->state()->CloneKnownNodeAspects(zone());

Darius Mercadier . resolved

```suggestion
      while (block->is_edge_split_block()) {
        block = block->control_node()->Cast<Jump>()->target();
      }
      known_node_aspects_ = block->state()->CloneKnownNodeAspects(zone());
```

Victor Gomes

I'm concerned that this approach shadows the `block` variable. Reusing the name creates ambiguity, as `block` already refers to the block we are currently visiting. One might one day add some code afterwards thinking that `block` is the current block.

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

satisfied_requirement

open

diffy

Darius Mercadier (Gerrit)

unread,

Sep 26, 2025, 5:17:55 AM (yesterday) Sep 26

to Victor Gomes, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

Attention needed from Victor Gomes

Darius Mercadier added 1 comment

File src/maglev/maglev-kna-processor.h

Line 63, Patchset 1 (Latest):

      BasicBlock* next_block = block;
      while (next_block->is_edge_split_block()) {
        next_block = next_block->control_node()->Cast<Jump>()->target();
      }
      known_node_aspects_ = next_block->state()->CloneKnownNodeAspects(zone());

Darius Mercadier . resolved

```suggestion
      while (block->is_edge_split_block()) {
        block = block->control_node()->Cast<Jump>()->target();
      }
      known_node_aspects_ = block->state()->CloneKnownNodeAspects(zone());
```

Victor Gomes

I'm concerned that this approach shadows the `block` variable. Reusing the name creates ambiguity, as `block` already refers to the block we are currently visiting. One might one day add some code afterwards thinking that `block` is the current block.

Darius Mercadier

Ok sounds good :)

Open in Gerrit

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.

Gerrit

satisfied_requirement

open

diffy

Victor Gomes (Gerrit)

unread,

Sep 26, 2025, 5:57:54 AM (yesterday) Sep 26

to Darius Mercadier, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

Victor Gomes voted and added 1 comment

Votes added by Victor Gomes

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Victor Gomes . resolved

Thanks!

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

satisfied_requirement

open

diffy

V8 LUCI CQ (Gerrit)

unread,

Sep 26, 2025, 5:59:16 AM (yesterday) Sep 26

to Victor Gomes, Darius Mercadier, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org

V8 LUCI CQ submitted the change

Change information

Commit message:

[maglev] Clone next block KNA for split edge blocks

We might need a KNA when visiting the graph after Phi untagging,
since it can introduce conversion nodes in the split
edge block.

Bug: 424157317

Change-Id: I4961cb7bd1a3ad3970b68d4cefeab3d87244e5b5

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

Auto-Submit: Victor Gomes <victo...@chromium.org>

Reviewed-by: Darius Mercadier <dmerc...@chromium.org>

Commit-Queue: Victor Gomes <victo...@chromium.org>

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

Files:

M src/maglev/maglev-kna-processor.h

Change size: S

Delta: 1 file changed, 9 insertions(+), 7 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Darius Mercadier

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[maglev] Clone next block KNA for split edge blocks [v8/v8 : main]

Victor Gomes (Gerrit)

Victor Gomes voted and added 1 comment

Votes added by Victor Gomes

1 comment

Related details

Darius Mercadier (Gerrit)

Darius Mercadier voted and added 2 comments

Votes added by Darius Mercadier

2 comments

Related details

Victor Gomes (Gerrit)

Victor Gomes added 2 comments

Related details

Darius Mercadier (Gerrit)

Darius Mercadier added 1 comment

Related details

Victor Gomes (Gerrit)

Victor Gomes voted and added 1 comment

Votes added by Victor Gomes

1 comment

Related details

V8 LUCI CQ (Gerrit)

V8 LUCI CQ submitted the change

Change information