Bug 140233 – Add the lexicalEnvironment as an operand to op_get_argument_by_val

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

Created attachment 244230 [details]
the patch.

[Mark Lam]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

(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.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 244238 [details]
patch 2

[WebKit Commit Bot]

Comment on attachment 244238 [details]
patch 2

Clearing flags on attachment: 244238

Committed r178106: <http://trac.webkit.org/changeset/178106>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.