Bug 142071

Summary:

BytecodeGenerator::constLocal() behaves identically to BytecodeGenerator::local() for the purposes of its one caller

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

barraclough, benjamin, bfulgham, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, saam, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

141174

Attachments:

Description	Flags
the patch	mark.lam: review+
the patch	benjamin: review+

Description Filip Pizlo 2015-02-26 19:16:40 PST

So, remove constLocal().

The behavioral differences are:

- constLocal() doesn't have a special case for "this" that overrides other checks like the shouldOptimizeLocals() check.  But the one user of constLocal() is for the "const x" expression, and "const this" doesn't parse.

- constLocal() won't createArgumentsIfNecessary() for "arguments".  But it's harmless if it does, since its one user assigns to the local.

So, we can remove constLocal() and make its one caller use local() instead.

Comment 1 Filip Pizlo 2015-02-26 19:17:43 PST

Created attachment 247483 [details]
the patch

Comment 2 Mark Lam 2015-02-26 19:39:09 PST

Comment on attachment 247483 [details]
the patch

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

Missing ChangeLog.  The text in your bugzilla description is good to have in the ChangeLog.

r=me with ChangeLog added.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4
> +*  Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012, 2013, 2015 Apple Inc. All rights reserved.

nit: You can collapse this to:
 * Copyright (C) 2003-2009, 2012-2013, 2015 Apple Inc. All rights reserved.

Comment 3 Filip Pizlo 2015-02-26 19:41:54 PST

Created attachment 247484 [details]
the patch

And now, with a ChangeLog!

Comment 4 Filip Pizlo 2015-02-26 21:13:20 PST

Landed in http://trac.webkit.org/changeset/180723

Comment 5 Alexey Proskuryakov 2015-02-26 22:09:39 PST

This broke Windows tests:

** The following JSC stress test failures have been introduced:
	stress/const-arguments.js.always-trigger-copy-phase
	stress/const-arguments.js.default
	stress/const-arguments.js.dfg-eager
	stress/const-arguments.js.dfg-eager-no-cjit-validate
	stress/const-arguments.js.no-cjit-validate-phases
	stress/const-arguments.js.no-llint

Comment 6 Filip Pizlo 2015-02-26 23:12:16 PST

(In reply to comment #5)
> This broke Windows tests:
> 
> ** The following JSC stress test failures have been introduced:
> 	stress/const-arguments.js.always-trigger-copy-phase
> 	stress/const-arguments.js.default
> 	stress/const-arguments.js.dfg-eager
> 	stress/const-arguments.js.dfg-eager-no-cjit-validate
> 	stress/const-arguments.js.no-cjit-validate-phases
> 	stress/const-arguments.js.no-llint

It might have broken other things too.  I'm looking.

Comment 7 Brent Fulgham 2015-02-26 23:19:08 PST

This broke the iOS build and Windows debug tests.

This should be rolled out if it cannot be fixed ASAP.

Comment 8 Filip Pizlo 2015-02-26 23:23:26 PST

(In reply to comment #7)
> This broke the iOS build and Windows debug tests.
> 
> This should be rolled out if it cannot be fixed ASAP.

I'm also seeing lots of breakage.  I agree with rollout.

Comment 9 Filip Pizlo 2015-02-26 23:27:02 PST

Rolled out in http://trac.webkit.org/changeset/180732

Comment 10 Filip Pizlo 2015-02-27 07:33:13 PST

It looks like there is some bizarre behavior here.  I'll have to address it a different way in my refactoring in bug 141174.