[baseline] Optimize ToBooleanForBaselineJump for static roots [v8/v8 : main]
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Auto-Submit | +1 |
1 comment
PTAL, followup to the other one to avoid redundant checks.
Related details
- Victor Gomes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Code-Review | +1 |
2 comments
LGTM + suggestion.
TNode<Object> value, bool skip_smi_and_static_root_check, Label* if_true,This is a personal opinion, but I think booleans that split the control flow like that makes it hard to follow.
I'd rather see a `BranchIfToBooleanIsTrue(T<Object>, if_true, if_false)` and
`BranchIfToBooleanIsTrueNonOddballs(T<HeapObject>, ....)`
and then called in the BaselineCompiler.
Not a strong opinion, though.
Related details
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
TNode<Object> value, bool skip_smi_and_static_root_check, Label* if_true,This is a personal opinion, but I think booleans that split the control flow like that makes it hard to follow.
I'd rather see a `BranchIfToBooleanIsTrue(T<Object>, if_true, if_false)` and
`BranchIfToBooleanIsTrueNonOddballs(T<HeapObject>, ....)`and then called in the BaselineCompiler.
Not a strong opinion, though.
I see your point, but it's a bit tricky with the additional `!V8_STATIC_ROOTS_BOOL` path split where we actually don't check for `Null` or `Undefined`, relying instead on the undetectable map check for them (meaning that `BranchIfToBooleanIsTrueNonOddballs` would have to carve out an exception for `Null`/`Undefined`). I also kind of like the duality here that we either branch or DCHECK on the checks that we can skip, maybe I can make that clearer by sharing more code between the branching vs DCHECKing paths?
Related details
- Victor Gomes
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)
Victor Gomes added 1 comment![]( "Open in Gerrit")
TNode<Object> value, bool skip_smi_and_static_root_check, Label* if_true,Leszek SwirskiThis is a personal opinion, but I think booleans that split the control flow like that makes it hard to follow.
I'd rather see a `BranchIfToBooleanIsTrue(T<Object>, if_true, if_false)` and
`BranchIfToBooleanIsTrueNonOddballs(T<HeapObject>, ....)`and then called in the BaselineCompiler.
Not a strong opinion, though.
I see your point, but it's a bit tricky with the additional `!V8_STATIC_ROOTS_BOOL` path split where we actually don't check for `Null` or `Undefined`, relying instead on the undetectable map check for them (meaning that `BranchIfToBooleanIsTrueNonOddballs` would have to carve out an exception for `Null`/`Undefined`). I also kind of like the duality here that we either branch or DCHECK on the checks that we can skip, maybe I can make that clearer by sharing more code between the branching vs DCHECKing paths?
Fair enough. As I said, not very strong opinion here, since this is still a relatively simple CSA function.
Related details
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Commit-Queue | +2 |
1 comment
TNode<Object> value, bool skip_smi_and_static_root_check, Label* if_true,Leszek SwirskiThis is a personal opinion, but I think booleans that split the control flow like that makes it hard to follow.
I'd rather see a `BranchIfToBooleanIsTrue(T<Object>, if_true, if_false)` and
`BranchIfToBooleanIsTrueNonOddballs(T<HeapObject>, ....)`and then called in the BaselineCompiler.
Not a strong opinion, though.
Victor GomesI see your point, but it's a bit tricky with the additional `!V8_STATIC_ROOTS_BOOL` path split where we actually don't check for `Null` or `Undefined`, relying instead on the undetectable map check for them (meaning that `BranchIfToBooleanIsTrueNonOddballs` would have to carve out an exception for `Null`/`Undefined`). I also kind of like the duality here that we either branch or DCHECK on the checks that we can skip, maybe I can make that clearer by sharing more code between the branching vs DCHECKing paths?
Fair enough. As I said, not very strong opinion here, since this is still a relatively simple CSA function.
Then I'll leave as-is because I like the DCHECKs (in case I screw up when the skipping is possible and when it isn't) 😊
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[baseline] Optimize ToBooleanForBaselineJump for static roots
Update BranchIfToBooleanIsTrue to optionally skip Smi and static root
checks. Configure ToBooleanForBaselineJump to skip these checks when
V8_STATIC_ROOTS is enabled, as they are already handled inline by
the baseline compiler.
- M src/builtins/base.tq
- M src/builtins/conversion.tq
- M src/codegen/code-stub-assembler.cc
- M src/codegen/code-stub-assembler.h
Code-Review: +1 by Victor Gomes
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |