[maglev] Better hole static check elimination [v8/v8 : main]
Victor Gomes (Gerrit)
Victor Gomes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Victor Gomes
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Related details
- Marja Hölttä
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted and added 3 comments![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Code-Review | +1 |
3 comments
lgtm
case Opcode::kLoadFixedArrayElement:Should kLoadHoleyFixedDoubleArrayElement be here, too?
switch (opcode) {I'm already seeing somebody forgetting to update this list! Is there anything we can do to make this more robust and/or make fuzzers catch problems faster? I guess there's no central bottleneck where we could check that if a ValueNode* is the hole, then its opcode must be one of these? Is there anything else we can do to force people adding new opcodes remember this? (Aside from removing the default branch and adding all opcodes explicitly, which is... maybe overkill?)
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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Auto-Submit | +1 |
| Commit-Queue | +2 |
3 comments
Should kLoadHoleyFixedDoubleArrayElement be here, too?
No, holey doubles are not a proper hole. They are treated as undefined.
I'm already seeing somebody forgetting to update this list! Is there anything we can do to make this more robust and/or make fuzzers catch problems faster? I guess there's no central bottleneck where we could check that if a ValueNode* is the hole, then its opcode must be one of these? Is there anything else we can do to force people adding new opcodes remember this? (Aside from removing the default branch and adding all opcodes explicitly, which is... maybe overkill?)
Yes, I don't actually know. If you have a better idea, I'm all ears.
I think removing the default branch is indeed overkill.
I thought about adding a property OpProperties::CanBeTheHole(), but the problem kind of remains.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted and added 1 comment![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Code-Review | +1 |
1 comment
switch (opcode) {Victor GomesI'm already seeing somebody forgetting to update this list! Is there anything we can do to make this more robust and/or make fuzzers catch problems faster? I guess there's no central bottleneck where we could check that if a ValueNode* is the hole, then its opcode must be one of these? Is there anything else we can do to force people adding new opcodes remember this? (Aside from removing the default branch and adding all opcodes explicitly, which is... maybe overkill?)
Yes, I don't actually know. If you have a better idea, I'm all ears.
I think removing the default branch is indeed overkill.
I thought about adding a property OpProperties::CanBeTheHole(), but the problem kind of remains.
(For a follow-up)
Could add the properties CanBeTheHole and CannotBeTheHole and require that exactly one is set?
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 added 1 comment![]( "Open in Gerrit")
switch (opcode) {Victor GomesI'm already seeing somebody forgetting to update this list! Is there anything we can do to make this more robust and/or make fuzzers catch problems faster? I guess there's no central bottleneck where we could check that if a ValueNode* is the hole, then its opcode must be one of these? Is there anything else we can do to force people adding new opcodes remember this? (Aside from removing the default branch and adding all opcodes explicitly, which is... maybe overkill?)
Marja HölttäYes, I don't actually know. If you have a better idea, I'm all ears.
I think removing the default branch is indeed overkill.
I thought about adding a property OpProperties::CanBeTheHole(), but the problem kind of remains.
(For a follow-up)
Could add the properties CanBeTheHole and CannotBeTheHole and require that exactly one is set?
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 added 1 comment![]( "Open in Gerrit")
switch (opcode) {Victor GomesI'm already seeing somebody forgetting to update this list! Is there anything we can do to make this more robust and/or make fuzzers catch problems faster? I guess there's no central bottleneck where we could check that if a ValueNode* is the hole, then its opcode must be one of these? Is there anything else we can do to force people adding new opcodes remember this? (Aside from removing the default branch and adding all opcodes explicitly, which is... maybe overkill?)
Marja HölttäYes, I don't actually know. If you have a better idea, I'm all ears.
I think removing the default branch is indeed overkill.
I thought about adding a property OpProperties::CanBeTheHole(), but the problem kind of remains.
(For a follow-up)
Could add the properties CanBeTheHole and CannotBeTheHole and require that exactly one is set?
Maybe we can also emit some debug-code verification for the node's result in codegen
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
[maglev] Better hole static check elimination
- M src/maglev/maglev-graph-builder.cc
- M src/maglev/maglev-graph-optimizer.cc
- M src/maglev/maglev-graph-optimizer.h
- M src/maglev/maglev-ir.cc
- M src/maglev/maglev-ir.h
Code-Review: +1 by Marja Hölttä
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
// Avoid the check if {value} has an alternative whose representation doesn't
// allow the hole.
if (const NodeInfo* info = known_node_aspects().TryGetInfoFor(value)) {
auto& alt = info->alternative();
if (alt.int32() || alt.truncated_int32_to_number() || alt.float64()) {
return ReduceResult::Done();
}
}Removing this seems like a regression, or am I missing something?
Related details
- Victor Gomes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |