Bug 215434 – OSRAvailabilityAnalysis shouldn't mark GetStack nodes directly as valid places for recovery

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

OSRAvailabilityAnalysis shouldn't mark GetStack nodes directly as valid places for recovery

[Keith Miller]

Created attachment 406484 [details]
Patch

[Keith Miller]

Created attachment 406485 [details]
Patch

[Saam Barati]

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

[Keith Miller]

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.

[Keith Miller]

rdar://65218346

[Keith Miller]

Landed in r265685