| Summary: | OSRAvailabilityAnalysis shouldn't mark GetStack nodes directly as valid places for recovery | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Keith Miller
2020-08-12 17:30:17 PDT
Created attachment 406484 [details]
Patch
Created attachment 406485 [details]
Patch
Comment on attachment 406485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406485&action=review Nice. r=me > Source/JavaScriptCore/ChangeLog:9 > + It's somewhat subtle that we cannot mark the node in Availability > + for GetStack. The reason is that if we did we would need to make nit: this first sentence could use a touch of clarification around the GetStack itself being the availability's node field > Source/JavaScriptCore/ChangeLog:12 > + if you had a graph like (after PutStack sinking): FWIW, this graph would be valid even if it happened without put stack sinking. Not sure if it needs specification. > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:136 > + // FIXME: The mayExit status of a node doesn't seem like it should mean we don't need to have everything available. It doesn't. I wouldn't write "seem like" here Please file a bug and link to it here > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:137 > + if (mayExit(m_graph, node) != DoesNotExit && node->origin.exitOK) { nit: Not sure you need exitOK here. Probably can be an assert I'm guessing. Might not matter much, given the above fixme is to remove this mayExit call > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:167 > + // FIXME: It seems like we should be able to do at least some validation when OSR entering. Why can't we? Also, link to a bug here please. > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:197 > +void validateOSRExitAvailability(Graph& graph) nice! > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:236 > + // It's somewhat subtle that we cannot use node for GetStack. The reason is that if we did we would need to make any phase that converts "use the node for GetStack" is kinda not detailed enough here. I'd say something along the lines of "We cannot use the node of the GetStack itself in the availability's node field" > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.h:44 > +// Unlike the phase above this function doesn't mutate the graph's BasicBlock SSA metadata. Also, does nothing if !validationEnabled() nice > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.h:-50 > - revert Comment on attachment 406485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406485&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + for GetStack. The reason is that if we did we would need to make > > nit: this first sentence could use a touch of clarification around the GetStack itself being the availability's node field Yeah, I wasn't sure how to phrase that better. It's kinda hard to describe... >> Source/JavaScriptCore/ChangeLog:12 >> + if you had a graph like (after PutStack sinking): > > FWIW, this graph would be valid even if it happened without put stack sinking. Not sure if it needs specification. Oh, I added that to explain how we could end up with this graph. Not that it's the only way. >> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:136 >> + // FIXME: The mayExit status of a node doesn't seem like it should mean we don't need to have everything available. > > It doesn't. I wouldn't write "seem like" here > > Please file a bug and link to it here Done. >> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:137 >> + if (mayExit(m_graph, node) != DoesNotExit && node->origin.exitOK) { > > nit: Not sure you need exitOK here. Probably can be an assert I'm guessing. > > Might not matter much, given the above fixme is to remove this mayExit call You could have a node where mayExit says it could exit but exitOK is false. We probably shouldn't say it mayExit but that's the current semantics. >> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:167 >> + // FIXME: It seems like we should be able to do at least some validation when OSR entering. > > Why can't we? > Also, link to a bug here please. I just copied that from the FTL lowering code. I didn't bother to figure out why that was the case. >> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:236 >> + // It's somewhat subtle that we cannot use node for GetStack. The reason is that if we did we would need to make any phase that converts > > "use the node for GetStack" is kinda not detailed enough here. I'd say something along the lines of "We cannot use the node of the GetStack itself in the availability's node field" Sure, I'll change this and the changelog. |