Victor Gomes (Gerrit)

unread,

6:35 AM (5 hours ago) 6:35 AM

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,

6:58 AM (4 hours ago) 6:58 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

Darius Mercadier added 7 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Darius Mercadier . unresolved

I don't feel super comfortable landing this without any checking that the types are correct..

File src/maglev/maglev-ir.h

Line 10197, Patchset 1 (Latest): V(RegExpPrototypeExec, UnionType(NodeType::kJSArray, NodeType::kNull)) \

Darius Mercadier . unresolved

Are you sure?

Line 10136, Patchset 1 (Latest): V(StringPrototypeLastIndexOf, NodeType::kNumber) \

Darius Mercadier . unresolved

Same here, could be kSmi

Line 10135, Patchset 1 (Latest): V(StringPrototypeIndexOf, NodeType::kNumber) \

Darius Mercadier . unresolved

kSmi if you want to be more precise

Line 10131, Patchset 1 (Latest): V(StringPrototypeCodePointAt, NodeType::kSmi) \

Darius Mercadier . unresolved

No, this can also return Undefined.

Line 10129, Patchset 1 (Latest): V(StringPrototypeCharCodeAt, NodeType::kSmi) \

Darius Mercadier . unresolved

Number I think

Line 10126, Patchset 1 (Latest): V(SymbolPrototypeToString, NodeType::kSymbol) \

Darius Mercadier . unresolved

String

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,

7:19 AM (4 hours ago) 7:19 AM

to V8 LUCI CQ, 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 8 comments

Votes added by Victor Gomes

Auto-Submit

+1

8 comments

Patchset-level comments

File-level comment, Patchset 1:

Victor Gomes . resolved

PTAL again!

File-level comment, Patchset 1:

Darius Mercadier . resolved

I don't feel super comfortable landing this without any checking that the types are correct..

Victor Gomes

😊 It is inspired by https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/turbofan-typer.cc;drc=59748dc2385e4231d35fa3cac600e4f19d7c4ddb;l=1862

File src/maglev/maglev-ir.h

Line 10197, Patchset 1: V(RegExpPrototypeExec, UnionType(NodeType::kJSArray, NodeType::kNull)) \

Darius Mercadier . unresolved

Are you sure?

Victor Gomes

Followed TF here:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/turbofan-typer.cc;drc=59748dc2385e4231d35fa3cac600e4f19d7c4ddb;l=2084

Line 10136, Patchset 1: V(StringPrototypeLastIndexOf, NodeType::kNumber) \

Darius Mercadier . resolved

Same here, could be kSmi

Victor Gomes

Done

Line 10135, Patchset 1: V(StringPrototypeIndexOf, NodeType::kNumber) \

Darius Mercadier . resolved

kSmi if you want to be more precise

Victor Gomes

Done

Line 10131, Patchset 1: V(StringPrototypeCodePointAt, NodeType::kSmi) \

Darius Mercadier . resolved

No, this can also return Undefined.

Victor Gomes

Done

Line 10129, Patchset 1: V(StringPrototypeCharCodeAt, NodeType::kSmi) \

Darius Mercadier . resolved

Number I think

Victor Gomes

I guess it can be NaN right, thanks...

Line 10126, Patchset 1: V(SymbolPrototypeToString, NodeType::kSymbol) \

Darius Mercadier . resolved

String

Victor Gomes

Oops...

Open in Gerrit

Related details

Attention is currently required from:

Darius Mercadier

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

Darius Mercadier (Gerrit)

unread,

7:43 AM (4 hours ago) 7:43 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

Darius Mercadier added 2 comments

Patchset-level comments

File-level comment, Patchset 1:

Darius Mercadier . unresolved

I don't feel super comfortable landing this without any checking that the types are correct..

Victor Gomes

😊 It is inspired by https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/turbofan-typer.cc;drc=59748dc2385e4231d35fa3cac600e4f19d7c4ddb;l=1862

Darius Mercadier

Turbofan has `--assert-types` that will help find bugs around this code. Maglev/Turbolev doesn't have this :/

File src/maglev/maglev-ir.h

Line 10197, Patchset 1: V(RegExpPrototypeExec, UnionType(NodeType::kJSArray, NodeType::kNull)) \

Darius Mercadier . unresolved

Are you sure?

Victor Gomes

Followed TF here:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/turbofan-typer.cc;drc=59748dc2385e4231d35fa3cac600e4f19d7c4ddb;l=2084

Darius Mercadier

Would be nice to figure out if this can really be Null. If you don't have time to look deeper into it, then you can keep this Null add a TODO.

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,

8:00 AM (3 hours ago) 8:00 AM

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

Attention needed from Darius Mercadier

Victor Gomes added 2 comments

Patchset-level comments

File-level comment, Patchset 1:

Darius Mercadier . resolved

I don't feel super comfortable landing this without any checking that the types are correct..

Victor Gomes

😊 It is inspired by https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/turbofan-typer.cc;drc=59748dc2385e4231d35fa3cac600e4f19d7c4ddb;l=1862

Darius Mercadier

Turbofan has `--assert-types` that will help find bugs around this code. Maglev/Turbolev doesn't have this :/

Victor Gomes

Let's add that! We should add more verification passes before the finishing line. Follow up...

File src/maglev/maglev-ir.h

Line 10197, Patchset 1: V(RegExpPrototypeExec, UnionType(NodeType::kJSArray, NodeType::kNull)) \

Darius Mercadier . resolved

Are you sure?

Victor Gomes

Followed TF here:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/turbofan-typer.cc;drc=59748dc2385e4231d35fa3cac600e4f19d7c4ddb;l=2084

Darius Mercadier

Would be nice to figure out if this can really be Null. If you don't have time to look deeper into it, then you can keep this Null add a TODO.

Victor Gomes

Yes, it ca: https://262.ecma-international.org/16.0/index.html?_gl=1*1aqxv08*_ga*MTU0MTg0MzIzNy4xNzM0NjAzNjgz*_ga_TDCK4DWEPP*czE3NjgzMDkxNzkkbzQkZzAkdDE3NjgzMDkxNzkkajYwJGwwJGgw#sec-regexpbuiltinexec

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,

8:01 AM (3 hours ago) 8:01 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

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:

Darius Mercadier . resolved

I don't feel super comfortable landing this without any checking that the types are correct..

Victor Gomes

😊 It is inspired by https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/turbofan-typer.cc;drc=59748dc2385e4231d35fa3cac600e4f19d7c4ddb;l=1862

Darius Mercadier

Turbofan has `--assert-types` that will help find bugs around this code. Maglev/Turbolev doesn't have this :/

Victor Gomes

Let's add that! We should add more verification passes before the finishing line. Follow up...

Darius Mercadier

Sounds good to me :)

File-level comment, Patchset 2 (Latest):

Darius Mercadier . resolved

lgtm

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,

8:02 AM (3 hours ago) 8:02 AM

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 2 (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,

8:41 AM (3 hours ago) 8:41 AM

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] Return static type for builtins

Bug: 431933185

Change-Id: I3be844d74e4fb6786b35a698c241bad15e586054

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

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

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

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

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

Files:

M src/maglev/maglev-ir.h

Change size: M

Delta: 1 file changed, 170 insertions(+), 0 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] Return static type for builtins [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 added 7 comments

Related details

Victor Gomes (Gerrit)

Victor Gomes voted and added 8 comments

Votes added by Victor Gomes

8 comments

Related details

Darius Mercadier (Gerrit)

Darius Mercadier added 2 comments

Related details

Victor Gomes (Gerrit)

Victor Gomes added 2 comments

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 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