Bug 216110 - Finish comment describing the various *Stack SSA nodes in DFG
Summary: Finish comment describing the various *Stack SSA nodes in DFG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-02 17:33 PDT by Keith Miller
Modified: 2020-09-03 14:44 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.61 KB, patch)
2020-09-02 17:35 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-09-02 17:33:26 PDT
Finish comment describing the various *Stack SSA nodes in DFG
Comment 1 Keith Miller 2020-09-02 17:35:29 PDT
Created attachment 407839 [details]
Patch
Comment 2 EWS 2020-09-03 10:39:12 PDT
Committed r266532: <https://trac.webkit.org/changeset/266532>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407839 [details].
Comment 3 Radar WebKit Bug Importer 2020-09-03 10:40:18 PDT
<rdar://problem/68288007>
Comment 4 Saam Barati 2020-09-03 12:29:36 PDT
Comment on attachment 407839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407839&action=review

> Source/JavaScriptCore/dfg/DFGNodeType.h:79
> +    /* exits are rare this is preferable to representing availability directly in SSA. */\

why? Storing to the stack isn't free. If something is in a register, you don't also want to throw it onto the stack just for OSRs sake.
Comment 5 Keith Miller 2020-09-03 12:35:27 PDT
Comment on attachment 407839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407839&action=review

>> Source/JavaScriptCore/dfg/DFGNodeType.h:79
>> +    /* exits are rare this is preferable to representing availability directly in SSA. */\
> 
> why? Storing to the stack isn't free. If something is in a register, you don't also want to throw it onto the stack just for OSRs sake.

I don't think that's what this comment says but maybe it could be more explicit.
Comment 6 Saam Barati 2020-09-03 13:49:57 PDT
(In reply to Keith Miller from comment #5)
> Comment on attachment 407839 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407839&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGNodeType.h:79
> >> +    /* exits are rare this is preferable to representing availability directly in SSA. */\
> > 
> > why? Storing to the stack isn't free. If something is in a register, you don't also want to throw it onto the stack just for OSRs sake.
> 
> I don't think that's what this comment says but maybe it could be more
> explicit.

"this is preferable to representing availability directly in SSA" is not right.
Comment 7 Saam Barati 2020-09-03 13:50:25 PDT
(In reply to Saam Barati from comment #6)
> (In reply to Keith Miller from comment #5)
> > Comment on attachment 407839 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=407839&action=review
> > 
> > >> Source/JavaScriptCore/dfg/DFGNodeType.h:79
> > >> +    /* exits are rare this is preferable to representing availability directly in SSA. */\
> > > 
> > > why? Storing to the stack isn't free. If something is in a register, you don't also want to throw it onto the stack just for OSRs sake.
> > 
> > I don't think that's what this comment says but maybe it could be more
> > explicit.
> 
> "this is preferable to representing availability directly in SSA" is not
> right.

Maybe you mean if both things were already the case? Like we happened to have a spill for another reason?
Comment 8 Keith Miller 2020-09-03 14:44:16 PDT
Comment on attachment 407839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407839&action=review

>>>>> Source/JavaScriptCore/dfg/DFGNodeType.h:79
>>>>> +    /* exits are rare this is preferable to representing availability directly in SSA. */\
>>>> 
>>>> why? Storing to the stack isn't free. If something is in a register, you don't also want to throw it onto the stack just for OSRs sake.
>>> 
>>> I don't think that's what this comment says but maybe it could be more explicit.
>> 
>> "this is preferable to representing availability directly in SSA" is not right.
> 
> Maybe you mean if both things were already the case? Like we happened to have a spill for another reason?

Correct, I'm saying that the PutStacks are used to represent shadow data flow. We don't necessarily want B3 spend time register allocating that data if we could get it from the stack. That comment wasn't trying to say it's preferable to PutStack every OSR exit value.