[maglev] Re-enable receiver/closure elision for non-lazy frame states [v8/v8 : main]
Jakob Linke (Gerrit)
Jakob Linke added 1 comment![]( "Open in Gerrit")
Related details
- Darius Mercadier
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 Code-Review+1![]( "Open in Gerrit")
- Darius Mercadier
- Jakob Linke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 2 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
2 comments
lgtm % comment
*compilation_unit_,
[&](ValueNode* node, interpreter::Register) { AddDeoptUse(node); });
AddDeoptUse(entry_stack_check_frame_->closure());This frame is also used as lazy deopt frame for the FunctionEntryStackCheck, so probably you shouldn't change this.
Although in practice you really shouldn't see any vobjs here... Maybe the best would be to DCHECK that neither closure nor receiver are vobjs here.
Related details
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke added 1 comment![]( "Open in Gerrit")
*compilation_unit_,
[&](ValueNode* node, interpreter::Register) { AddDeoptUse(node); });
AddDeoptUse(entry_stack_check_frame_->closure());This frame is also used as lazy deopt frame for the FunctionEntryStackCheck, so probably you shouldn't change this.
Although in practice you really shouldn't see any vobjs here... Maybe the best would be to DCHECK that neither closure nor receiver are vobjs here.
Those are two separate concerns though. vobj: only relevant wrt recursively setting use_count_. Receiver/closure materialization matters in any case, no?
If this frame is used for lazy deopts, Imo we should keep the AddMaterializedDeoptUse special cases here.
Related details
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
*compilation_unit_,
[&](ValueNode* node, interpreter::Register) { AddDeoptUse(node); });
AddDeoptUse(entry_stack_check_frame_->closure());Jakob LinkeThis frame is also used as lazy deopt frame for the FunctionEntryStackCheck, so probably you shouldn't change this.
Although in practice you really shouldn't see any vobjs here... Maybe the best would be to DCHECK that neither closure nor receiver are vobjs here.
Those are two separate concerns though. vobj: only relevant wrt recursively setting use_count_. Receiver/closure materialization matters in any case, no?
If this frame is used for lazy deopts, Imo we should keep the AddMaterializedDeoptUse special cases here.
Receiver/closure materialization matters in any case, no?
Except that here reciever & closure shouldn't be dematerializable (ie, they can't be InlinedAllocation).
And so I think that a DCHECK that they aren't InlinedAllocation (rather than vobj as I initially said) makes more sense to document the assumptions that we're making and the state that we're in.
Related details
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jakob Linke
| Auto-Submit | +1 |
[&](ValueNode* node, interpreter::Register) { AddDeoptUse(node); });
AddDeoptUse(entry_stack_check_frame_->closure());Jakob LinkeThis frame is also used as lazy deopt frame for the FunctionEntryStackCheck, so probably you shouldn't change this.
Although in practice you really shouldn't see any vobjs here... Maybe the best would be to DCHECK that neither closure nor receiver are vobjs here.
Darius MercadierThose are two separate concerns though. vobj: only relevant wrt recursively setting use_count_. Receiver/closure materialization matters in any case, no?
If this frame is used for lazy deopts, Imo we should keep the AddMaterializedDeoptUse special cases here.
Receiver/closure materialization matters in any case, no?
Except that here reciever & closure shouldn't be dematerializable (ie, they can't be InlinedAllocation).
And so I think that a DCHECK that they aren't InlinedAllocation (rather than vobj as I initially said) makes more sense to document the assumptions that we're making and the state that we're in.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Jakob Linke
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] Re-enable receiver/closure elision for non-lazy frame states
Only lazy frame states are used for computing stack traces, from which
.getThis and .getFunction may access the receiver and closure.
This is a followup to https://crrev.com/c/7642804.
- M src/maglev/maglev-graph-builder.cc
Code-Review: +1 by Victor Gomes, +1 by Darius Mercadier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |