[webkit-reviews] review granted: [Bug 215434] OSRAvailabilityAnalysis shouldn't mark GetStack nodes directly as valid places for recovery : [Attachment 406485] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 215434: OSRAvailabilityAnalysis shouldn't mark GetStack nodes directly as
valid places for recovery
https://bugs.webkit.org/show_bug.cgi?id=215434

Attachment 406485: Patch

https://bugs.webkit.org/attachment.cgi?id=406485&action=review




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 406485
  --> https://bugs.webkit.org/attachment.cgi?id=406485
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