Bug 140233 - Add the lexicalEnvironment as an operand to op_get_argument_by_val
Summary: Add the lexicalEnvironment as an operand to op_get_argument_by_val
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 140148
Blocks: 140097 140236
  Show dependency treegraph
 
Reported: 2015-01-07 17:06 PST by Mark Lam
Modified: 2015-01-07 22:20 PST (History)
5 users (show)

See Also:


Attachments
the patch. (12.98 KB, patch)
2015-01-07 17:43 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch 2 (5.84 KB, patch)
2015-01-07 21:14 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-01-07 17:06:50 PST
This is another step towards having the DFG CreateArguments and GetMyArgumentByVal nodes take the lexicalEnvironment as a parameter.

Also changed the default value of CodeBlock::lexicalEnvironment() to a constant 0 VirtualRegister instead of an invalid one.  This makes the BytecodeGenerator code that loads it as an operand slightly more readable.
Comment 1 Mark Lam 2015-01-07 17:43:14 PST
Created attachment 244230 [details]
the patch.
Comment 2 Mark Lam 2015-01-07 19:57:38 PST
The mac-wk2 EWS test run failure appears to be unrelated to this patch.  I've run the layout tests locally with this patch, and the results looks to be as fine.  I'm not getting any crashes.
Comment 3 Filip Pizlo 2015-01-07 20:11:57 PST
Comment on attachment 244230 [details]
the patch.

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

> Source/JavaScriptCore/ChangeLog:13
> +        Also changed the value of CodeBlock::lexicalEnvironment() to be
> +        a constant 0 VirtualRegister instead of an invalid one when there isn't
> +        a JSLexicalEnvironment present.  This makes the BytecodeGenerator code
> +        that loads it as an operand slightly more readable.

Can we not do this?  It's weird to have "isLocal()" checks in the DFG.
Comment 4 Filip Pizlo 2015-01-07 20:12:18 PST
(In reply to comment #3)
> Comment on attachment 244230 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244230&action=review
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        Also changed the value of CodeBlock::lexicalEnvironment() to be
> > +        a constant 0 VirtualRegister instead of an invalid one when there isn't
> > +        a JSLexicalEnvironment present.  This makes the BytecodeGenerator code
> > +        that loads it as an operand slightly more readable.
> 
> Can we not do this?  It's weird to have "isLocal()" checks in the DFG.

... its' also just inconsistent with how we do things elsewhere.
Comment 5 Mark Lam 2015-01-07 20:50:45 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 244230 [details]
> > the patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=244230&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:13
> > > +        Also changed the value of CodeBlock::lexicalEnvironment() to be
> > > +        a constant 0 VirtualRegister instead of an invalid one when there isn't
> > > +        a JSLexicalEnvironment present.  This makes the BytecodeGenerator code
> > > +        that loads it as an operand slightly more readable.
> > 
> > Can we not do this?  It's weird to have "isLocal()" checks in the DFG.
> 
> ... its' also just inconsistent with how we do things elsewhere.

Ok, I’ll remove that part.
Comment 6 Mark Lam 2015-01-07 21:14:03 PST
Created attachment 244238 [details]
patch 2
Comment 7 WebKit Commit Bot 2015-01-07 22:20:16 PST
Comment on attachment 244238 [details]
patch 2

Clearing flags on attachment: 244238

Committed r178106: <http://trac.webkit.org/changeset/178106>
Comment 8 WebKit Commit Bot 2015-01-07 22:20:20 PST
All reviewed patches have been landed.  Closing bug.