Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(37)

Issue 7553006: Check for phi-uses of arguments object before eliminating dead phi's. (Closed)

Created:
9 years, 4 months ago by Vyacheslav Egorov (Chromium)
Modified:
9 years, 4 months ago
CC:
v8-dev
Visibility:
Public.

Description

Check for phi-uses of arguments object before eliminating dead phi's. HGraphBuilder::TryArgumentsAccess does not emit any uses for receiver and will generate incorrect code when receiver for a property access is defined by a phi that returns either arguments object or something else. BUG=v8:1582 TEST=test/mjsunit/regress/regress-1582.js Committed: http://code.google.com/p/v8/source/detail?r=8774

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -2 lines) Patch
M src/hydrogen.h View 1 chunk +4 lines, -0 lines 0 comments Download
M src/hydrogen.cc View 3 chunks +17 lines, -2 lines 2 comments Download
A test/mjsunit/regress/regress-1582.js View 1 chunk +47 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Vyacheslav Egorov (Chromium)
9 years, 4 months ago (2011-08-02 09:08:29 UTC) #1
Kevin Millikin (Chromium)
LGTM. http://codereview.chromium.org/7553006/diff/1/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/7553006/diff/1/src/hydrogen.cc#newcode2315 src/hydrogen.cc:2315: Bailout("Unsupported phi-use"); 1. There should be a unique ...
9 years, 4 months ago (2011-08-02 09:21:25 UTC) #2
Vyacheslav Egorov (Chromium)
9 years, 4 months ago (2011-08-02 09:26:23 UTC) #3
Thanks.

http://codereview.chromium.org/7553006/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7553006/diff/1/src/hydrogen.cc#newcode2315
src/hydrogen.cc:2315: Bailout("Unsupported phi-use");
1. Done.
2. I don't think HasStackOverflow is pretty enough I will keep bailout here.

On 2011/08/02 09:21:25, Kevin Millikin wrote:
> 1.  There should be a unique bailout reason (to distinguish from the later
one),
> I suggest "Unsupported phi use of arguments object" here and "Unsupported phi
> use of uninitialized constant" later.
> 
> 2. You could consider moving the calls to Bailout into the
CheckPhis/CollectPhis
> functions, make them void functions, and call them like:
> 
> graph()->CheckPhis();
> if (HasStackOverflow()) return NULL;
> 
> I'm not sure if I like that better.  What do you think?

Powered by Google App Engine
This is Rietveld 408576698