Simple support for const variables in Crankshaft....

Simple support for const variables in Crankshaft.

The approach is to handle the common case in the optimizing
compiler and to bailout for the rare corner cases.

This is done by initializing all local const-variables with
the hole value and disallowing any use of the hole value statically.
Committed: http://code.google.com/p/v8/source/detail?r=8104

[Kevin Millikin (Chromium), 2010-12-21 16:10:19]

This needs to disallow more things.  It doesn't correctly handle 

function f() { const x = 42; x += 1; return x; }

==> returns 43, should be 42.  I think you just forgot to treat compound
assignments.  It also doesn't handle

function f() { var x = 42; while (true) { const y = x; if (--x == 0) return y;
}}

==> returns 1, should return 42.

[fschneider, 2010-12-22 13:41:41]

Uploaded a new version.

Now const-initializations in the optimizer are more restricted to handle the
case of const inside loops.

[Kevin Millikin (Chromium), 2011-01-04 13:46:33]

I don't really like our const semantics, but I think we should find a way to
support a bit more of it than this change does.

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode2956
src/hydrogen.cc:2956: BAILOUT("unsupported reference to const variable");
ast_context()->ReturnValue(graph()->GetConstantUndefined());

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode3377
src/hydrogen.cc:3377: if (var->mode() == Variable::CONST) BAILOUT("unsupported
const assignment");
Instead of statically bailing out, how about visiting the operation and then
guarding the assignment with something like

if (var->mode() != Variable::CONST) ....

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode3484
src/hydrogen.cc:3484: BAILOUT("unsupported const assignment");
Instead of statically bailing out for non-INIT_CONST assignments to CONST
variables, you could just ignore them.

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode3488
src/hydrogen.cc:3488: HValue* old_value = environment()->Lookup(var);
It's a little indirect to add this use and detect it in a postprocessing pass. 
It seems more direct (though a bit more implementation effort) to keep track of
a loop nesting depth and bailout if non-zero.

I'm also not thrilled with InitConst as an instruction.  It's not initializing
anything and it's odd that it refers to the old value and not the new.  It's
more like UseValue or something.

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode4565
src/hydrogen.cc:4565: if (var->mode() == Variable::CONST) BAILOUT("unsupported
count operation");
Same comment.

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode4920
src/hydrogen.cc:4920: // functions, variables with slot type LOOKUP
Fix the comment so it's still a sentence (add "and" and a period).

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode4929
src/hydrogen.cc:4929: if (var->mode() == Variable::CONST) {
I don't think you should change the test against decl->mode() to one against
var->mode().  Declaration::mode() can only be VAR or CONST, while
Variable::mode() could be all kinds of other stuff (DYNAMIC, DYNAMIC_GLOBAL,
etc).

I'm worried that things will break as we support more stuff if we don't handle
constants more or less like the baseline compiler.

http://codereview.chromium.org/6026006/diff/15001/src/hydrogen.cc#newcode4930
src/hydrogen.cc:4930: environment()->Bind(var, graph()->GetConstantHole());
I'm surprised you don't have to check that the variable is stack allocated here.

[Kevin Millikin (Chromium), 2011-05-30 10:50:34]

LGTM, small comments below.

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode2315
src/hydrogen.cc:2315: //if (scope->HasIllegalRedeclaration()) BAILOUT("illegal
redeclaration");
This should not be necessary, I think we check it in the caller of SetupScope.

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode3015
src/hydrogen.cc:3015: return Bailout("unsupported reference to const variable");
Let's make this say something like "reference to unitialized const variable"

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode3472
src/hydrogen.cc:3472: if (var->mode() == Variable::CONST) return
Bailout("unsupported const compound assignment");
Needs a line break before committing.

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode3582
src/hydrogen.cc:3582: return Bailout("unsupported const assignment");
Let's make the message something like "non-initializer assignment to const".

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode4904
src/hydrogen.cc:4904: if (var->mode() == Variable::CONST) return
Bailout("unsupported count operation");
Line break.

[fschneider, 2011-05-30 11:15:40]

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode2315
src/hydrogen.cc:2315: //if (scope->HasIllegalRedeclaration()) BAILOUT("illegal
redeclaration");
On 2011/05/30 10:50:34, Kevin Millikin wrote:
> This should not be necessary, I think we check it in the caller of SetupScope.

Done.

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode3015
src/hydrogen.cc:3015: return Bailout("unsupported reference to const variable");
On 2011/05/30 10:50:34, Kevin Millikin wrote:
> Let's make this say something like "reference to unitialized const variable"

Done.

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode3472
src/hydrogen.cc:3472: if (var->mode() == Variable::CONST) return
Bailout("unsupported const compound assignment");
On 2011/05/30 10:50:34, Kevin Millikin wrote:
> Needs a line break before committing.

Done.

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode3582
src/hydrogen.cc:3582: return Bailout("unsupported const assignment");
On 2011/05/30 10:50:34, Kevin Millikin wrote:
> Let's make the message something like "non-initializer assignment to const".

Done.

http://codereview.chromium.org/6026006/diff/21001/src/hydrogen.cc#newcode4904
src/hydrogen.cc:4904: if (var->mode() == Variable::CONST) return
Bailout("unsupported count operation");
On 2011/05/30 10:50:34, Kevin Millikin wrote:
> Line break.

Done.

[Rico, 2011-05-31 07:29:47]

http://codereview.chromium.org/6026006/diff/21005/test/mjsunit/compiler/regre...
File test/mjsunit/compiler/regress-const.js (right):

http://codereview.chromium.org/6026006/diff/21005/test/mjsunit/compiler/regre...
test/mjsunit/compiler/regress-const.js:44: f();
Could we use %OptimizeFunctionOnNextCall(f) instead of running this 1000000
times + below, this times out on arm sim novfp3

[Rico, 2011-05-31 07:35:13]

On 2011/05/31 07:29:47, Rico wrote:
>
http://codereview.chromium.org/6026006/diff/21005/test/mjsunit/compiler/regre...
> File test/mjsunit/compiler/regress-const.js (right):
> 
>
http://codereview.chromium.org/6026006/diff/21005/test/mjsunit/compiler/regre...
> test/mjsunit/compiler/regress-const.js:44: f();
> Could we use %OptimizeFunctionOnNextCall(f) instead of running this 1000000
> times + below, this times out on arm sim novfp3

Sorry, did not see that you already fixed this (stupid slow arm simulator :-))