Bug 142071 - BytecodeGenerator::constLocal() behaves identically to BytecodeGenerator::local() for the purposes of its one caller
Summary: BytecodeGenerator::constLocal() behaves identically to BytecodeGenerator::loc...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 141174
  Show dependency tree / graph
 
Reported: 2015-02-26 19:16 PST by Filip Pizlo
Modified: 2015-02-27 07:33 PST (History)
12 users (show)

See Also:

Attachments
the patch (3.80 KB, patch)
2015-02-26 19:17 PST, Filip Pizlo 		mark.lam: review+
Details | Formatted Diff | Diff
the patch (4.76 KB, patch)
2015-02-26 19:41 PST, Filip Pizlo 		benjamin: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.