Preliminary support for block contexts in hydrogen.

Preliminary support for block contexts in hydrogen.

Patch from Steven Keuchel <keuchel@chromium.org>;

BUG=v8:2198
LOG=N
TEST=mjsunit/harmony/block-let-crankshaft.js
R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21684

[ulan, 2014-05-28 10:28:15]

PTAL. If it looks good, I will port to other architectures.

[Michael Starzinger, 2014-06-03 23:12:21]

Just one drive-by-comment.

https://codereview.chromium.org/307593002/diff/20001/src/ia32/lithium-codegen...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/307593002/diff/20001/src/ia32/lithium-codegen...
src/ia32/lithium-codegen-ia32.cc:5673:
CallRuntime(Runtime::kHiddenPushBlockContext, 2, instr);
Since this is only a runtime call, do we really need a separate HInstruction for
it? Could we use HCallRuntime instead? Or are there any plans to optimize
HAllocateBlockContext in the near future?

[rossberg, 2014-06-04 12:54:54]

LGTM, mostly nits

https://codereview.chromium.org/307593002/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/307593002/diff/20001/src/ast.h#newcode444
src/ast.h:444: const BailoutId DeclsId() const { return decls_id_; }
nit: the first const here does not have any meaning.

https://codereview.chromium.org/307593002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/307593002/diff/20001/src/hydrogen.cc#newcode4283
src/hydrogen.cc:4283: set_scope(scope);
Hm, can pushing/popping the scope perhaps be factored into
BreakAndContinueScope, or a separate BlockScope abstraction?

https://codereview.chromium.org/307593002/diff/20001/src/ia32/lithium-codegen...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/307593002/diff/20001/src/ia32/lithium-codegen...
src/ia32/lithium-codegen-ia32.cc:5673:
CallRuntime(Runtime::kHiddenPushBlockContext, 2, instr);
On 2014/06/03 23:12:20, Michael Starzinger wrote:
> Since this is only a runtime call, do we really need a separate HInstruction
for
> it? Could we use HCallRuntime instead? Or are there any plans to optimize
> HAllocateBlockContext in the near future?

Perhaps not in the near future, but we will have to do it eventually, presumably
in the not so far future. That said, I don't have an opinion what to prefer
right now.

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
File test/mjsunit/harmony/block-let-crankshaft.js (right):

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
test/mjsunit/harmony/block-let-crankshaft.js:36: f27, f28, f29, f30, f31, f32,
f33 ];
Perhaps add a test for throwing out of a crankshaft'ed function inside a nested
block, just in case.

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
File test/mjsunit/harmony/block-scoping.js (right):

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
test/mjsunit/harmony/block-scoping.js:31: // TODO(ES6): properly activate
extended mode
Btw, we can remove this TODO now.

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
test/mjsunit/harmony/block-scoping.js:67: 
Why not assert optimization status here as well (and below)?

[ulan, 2014-06-04 14:02:04]

https://codereview.chromium.org/307593002/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/307593002/diff/20001/src/ast.h#newcode444
src/ast.h:444: const BailoutId DeclsId() const { return decls_id_; }
On 2014/06/04 12:54:53, rossberg wrote:
> nit: the first const here does not have any meaning.

Done.

https://codereview.chromium.org/307593002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/307593002/diff/20001/src/hydrogen.cc#newcode4283
src/hydrogen.cc:4283: set_scope(scope);
On 2014/06/04 12:54:53, rossberg wrote:
> Hm, can pushing/popping the scope perhaps be factored into
> BreakAndContinueScope, or a separate BlockScope abstraction?

Done.

https://codereview.chromium.org/307593002/diff/20001/src/ia32/lithium-codegen...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/307593002/diff/20001/src/ia32/lithium-codegen...
src/ia32/lithium-codegen-ia32.cc:5673:
CallRuntime(Runtime::kHiddenPushBlockContext, 2, instr);
Right, we will do it sooner than later. Leaving it as it is.

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
File test/mjsunit/harmony/block-let-crankshaft.js (right):

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
test/mjsunit/harmony/block-let-crankshaft.js:36: f27, f28, f29, f30, f31, f32,
f33 ];
On 2014/06/04 12:54:53, rossberg wrote:
> Perhaps add a test for throwing out of a crankshaft'ed function inside a
nested
> block, just in case.

Done.

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
File test/mjsunit/harmony/block-scoping.js (right):

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
test/mjsunit/harmony/block-scoping.js:31: // TODO(ES6): properly activate
extended mode
On 2014/06/04 12:54:53, rossberg wrote:
> Btw, we can remove this TODO now.

Done.

https://codereview.chromium.org/307593002/diff/20001/test/mjsunit/harmony/blo...
test/mjsunit/harmony/block-scoping.js:67: 
On 2014/06/04 12:54:53, rossberg wrote:
> Why not assert optimization status here as well (and below)?

Did it for the function below. This function doesn't get optimized because of
eval, so I removed %OptimizeFunctionOnNextCall for it.

[rossberg, 2014-06-04 15:30:22]

Still LGTM

[ulan, 2014-06-05 07:33:20]

Committed patchset #3 manually as r21684 (presubmit successful).