[Interpreter] Adds support for const/let variables to interpreter.

[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interpreter.

BUG=v8:4280, v8:4679
LOG=N

Committed: https://crrev.com/90721a51a35644b5dca8116895830b1d10629f68
Cr-Commit-Position: refs/heads/master@{#33819}

[mythria, 2016-01-26 12:31:58]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 12:32:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/1

[commit-bot: I haz the power, 2016-01-26 12:32:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-26 12:32:07]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mythria, 2016-01-26 13:08:33]

mythria@chromium.org changed reviewers:
+ mstarzinger@chromium.org, rmcilroy@chromium.org

[mythria, 2016-01-26 13:08:33]

PTAL.

Thanks,
Mythri

https://codereview.chromium.org/1634153002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1617: Handle<String> name) {
This is used for throwing when constant is re-assigned for parameters and
locals. I just did a minor optimization by moving loading the name into the
register outside the if-else statement. I am not sure we save much from it. If I
don't do that we could use the same function for variables in context slots or
local/parameters.

[mythria, 2016-01-26 13:16:39]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 13:16:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/1

[commit-bot: I haz the power, 2016-01-26 13:16:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-26 13:16:48]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mythria, 2016-01-26 13:18:50]

mythria@chromium.org changed reviewers:
+ oth@chromium.org

[mythria, 2016-01-26 13:18:50]

+orion

[oth, 2016-01-26 13:19:25]

The CQ bit was checked by oth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 13:19:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/1

[commit-bot: I haz the power, 2016-01-26 13:20:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-26 13:20:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/513)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/12855)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/14561)

[oth, 2016-01-26 13:29:00]

The CQ bit was checked by oth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 13:29:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/20001

[commit-bot: I haz the power, 2016-01-26 13:41:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-26 13:41:29]

Dry run: Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/10155)

[rmcilroy, 2016-01-26 14:37:28]

A couple of high-level comments. Not looked at everything yet.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1513:
builder()->JumpIfNotHole(&end_label).LoadUndefined().Bind(&end_label);
Newlines between builder operations (throughout).

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1634: void
BytecodeGenerator::BuildThrowReassignConstant(Handle<String> name) {
Could we merge both BuildThrowReassignConstant (and just always have the value
in a register)?

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1639: .LoadLiteral(name)
Move LoadLiteral / StoreAccumulatorInRegister(name_reg) before jump (to avoid
doing it twice) ?

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1670: } else if (mode == CONST && op !=
Token::INIT) {
As discussed, could you merge the code to do the hole check for this and the
context slot into a common helper function.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1675: // TODO(mythria): Find a test case
for strict mode legacy constants.
Legacy const doesn't exist for strict mode, right?

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1724: bool value_in_accumulator = true;
Let's not try to be smart with "value_in_accumulator" for now (particularly
since we will replace these by special bytecodes), let's just assume that it is
always in the accumulator (maintaining the LoadAccumulatorWithRegister in the
else branch) and have a new temporary register for the hole check which is
stored from the accumulator.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1759: // TODO(mythria): Find a test case
for strict mode legacy constants.
ditto.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1776: // assignment and restore back the
accumulator.
Not sure what "restore back the accumulator" means here?

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1791: // only variable being able to
trigger bind operations outside the TDZ
/s/variable being able/variable able

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:239: V(JumpIfNotHole, OperandType::kImm8)           
                             \
It seems dangerous to have these without Constant and ConstantWide variants of
this Bytecode as well. I'm assuming these aren't needed now due to the way these
jumps are used, and it's probably not worth adding them if we are going to add
explicit assignment+throw bytecodes. If so, please add a TODO that these should
be removed once we have these bytecodes, or that we should add wide variants).

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1631: // TODO(mythria): Implement with
wordNotEquals.
Any reason not to do that now?

[mythria, 2016-02-01 09:44:22]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 09:44:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/40001

[commit-bot: I haz the power, 2016-02-01 09:45:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-01 09:45:24]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/13081)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/14780)

[mythria, 2016-02-01 10:02:18]

PTAL

Thanks,
Mythri

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1513:
builder()->JumpIfNotHole(&end_label).LoadUndefined().Bind(&end_label);
On 2016/01/26 14:37:28, rmcilroy wrote:
> Newlines between builder operations (throughout).

I added newlines. cl format removes them. Shall I override cl format?

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1634: void
BytecodeGenerator::BuildThrowReassignConstant(Handle<String> name) {
On 2016/01/26 14:37:28, rmcilroy wrote:
> Could we merge both BuildThrowReassignConstant (and just always have the value
> in a register)?

Done.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1639: .LoadLiteral(name)
On 2016/01/26 14:37:28, rmcilroy wrote:
> Move LoadLiteral / StoreAccumulatorInRegister(name_reg) before jump (to avoid
> doing it twice) ?

For throwing const reassign error, we don't need the variable name. It was a bug
and I fixed it. So we need these only in one part of the if. 

Done.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1670: } else if (mode == CONST && op !=
Token::INIT) {
On 2016/01/26 14:37:28, rmcilroy wrote:
> As discussed, could you merge the code to do the hole check for this and the
> context slot into a common helper function.

Done. I did not pull const_legacy && Token::Init to helper function. Because, we
need to store to destination conditionally. We have to pass cotext_reg and
destination registers and only one of them would be valid. Let me know if you
have any suggestions.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1675: // TODO(mythria): Find a test case
for strict mode legacy constants.
On 2016/01/26 14:37:28, rmcilroy wrote:
> Legacy const doesn't exist for strict mode, right?

Done. Yes, it was a old comment. Retained the DCHECK because we are not totally
sure and have to check with andreas.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1724: bool value_in_accumulator = true;
On 2016/01/26 14:37:28, rmcilroy wrote:
> Let's not try to be smart with "value_in_accumulator" for now (particularly
> since we will replace these by special bytecodes), let's just assume that it
is
> always in the accumulator (maintaining the LoadAccumulatorWithRegister in the
> else branch) and have a new temporary register for the hole check which is
> stored from the accumulator.

Done.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1759: // TODO(mythria): Find a test case
for strict mode legacy constants.
On 2016/01/26 14:37:28, rmcilroy wrote:
> ditto.

Done.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1776: // assignment and restore back the
accumulator.
On 2016/01/26 14:37:28, rmcilroy wrote:
> Not sure what "restore back the accumulator" means here?

Done.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1791: // only variable being able to
trigger bind operations outside the TDZ
On 2016/01/26 14:37:28, rmcilroy wrote:
> /s/variable being able/variable able

Done.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:239: V(JumpIfNotHole, OperandType::kImm8)           
                             \
On 2016/01/26 14:37:28, rmcilroy wrote:
> It seems dangerous to have these without Constant and ConstantWide variants of
> this Bytecode as well. I'm assuming these aren't needed now due to the way
these
> jumps are used, and it's probably not worth adding them if we are going to add
> explicit assignment+throw bytecodes. If so, please add a TODO that these
should
> be removed once we have these bytecodes, or that we should add wide variants).

Done.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1631: // TODO(mythria): Implement with
wordNotEquals.
On 2016/01/26 14:37:28, rmcilroy wrote:
> Any reason not to do that now?

Done.

https://codereview.chromium.org/1634153002/diff/20001/test/mjsunit/mjsunit.st...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1634153002/diff/20001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:791: 'const-redecl': [SKIP],
These two fail because with is not supported.

[mythria, 2016-02-01 10:33:02]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 10:33:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/60001

[commit-bot: I haz the power, 2016-02-01 10:36:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-01 10:36:40]

Dry run: Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/10354)

[mythria, 2016-02-01 10:38:15]

Description was changed from

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interprter.

BUG=v8:4280,v8:4679
==========

to

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interprter.

BUG=v8:4280,v8:4679
LOG=N
==========

[mythria, 2016-02-02 11:03:59]

PTAL.

Thanks,
Mythri

[rmcilroy, 2016-02-03 12:28:27]

Looking much better now. A few more comments, but pretty much there.

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1513:
builder()->JumpIfNotHole(&end_label).LoadUndefined().Bind(&end_label);
On 2016/02/01 10:02:18, mythria wrote:
> On 2016/01/26 14:37:28, rmcilroy wrote:
> > Newlines between builder operations (throughout).
> 
> I added newlines. cl format removes them. Shall I override cl format?

I think we should override this if possible, it is pretty difficult to read
otherwise (another option would be to just call builder() at the start of every
line here)

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1602: BytecodeLabel end_label;
nit - add a TODO that we will replace this with a dedicated bytecode

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1613: BytecodeLabel end_label;
ditto

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1624: BytecodeLabel else_label;
ditto

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1636: VariableMode mode =
variable->mode();
DCHECK not legacy const mode

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1641: BuildThrowIfHole(variable->name());
nit - add comment

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1684: break;
nit - add comment on why this break is necessary

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1685: } else if (mode == CONST_LEGACY &&
op != Token::INIT) {
Could you drop this entire branch if you just made mode == CONST_LEGACY && op !=
Token::INIT not require a hole check (since the hole check doesn't do anything
in this case anyway).  You could add the DCHECK(!(mode == CONST_LEGACY && op !=
Token::INIT && is_strict(language_mode()))) up above next to the
hole_check_required boolean if you still want it.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1687:
builder()->LoadAccumulatorWithRegister(value_temp);
nit - add a comment

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1745: break;
ditto

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1747: DCHECK(!is_strict(language_mode()));
Same comment

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1775: } else if (mode == CONST_LEGACY &&
op != Token::INIT) {
ditto

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:2635: TEST(JumpOnHole) {
Could you replicate the test-interpreter tests here rather than testing for just
JumpOnHole, I think that would be more useful.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:78: i::FLAG_legacy_const =
true;
You can just set and restore these flags in the test itself rather than adding a
new MakeBytecode function.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:2368: U8(1),                 
               // 58
Fix indenting and remove "58"

Note: A way of indenting the operands is to use a non-line comment, i.e.:
B(CallRuntime), U16(Runtime::kThrowReferenceError), R(4),   //
/*           */ U(1),                                       //

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:2385: B(Ldar), R(4),         
               // 100
ditto

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7589: "const x = 10;",
Fix indentation

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7683: "let x = 10;",
Fix indentation

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:8100: }
Nice set of tests, thanks!

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:101: i::FLAG_legacy_const = true;
Again you can just do this in the test rather than creating another GetCallable

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3890: for (size_t i = 0; i <
arraysize(const_decl); i++) {
comment - tests for sloppy mode.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3928: for (size_t i = 0; i <
arraysize(const_decl); i++) {
ditto

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:4031: for (size_t i = 0; i <
arraysize(const_decl); i++) {
ditto

[mythria, 2016-02-04 10:28:53]

Thanks for your comments. I fixed them. PTAL. 

Thanks,
Mythri.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1602: BytecodeLabel end_label;
On 2016/02/03 12:28:26, rmcilroy wrote:
> nit - add a TODO that we will replace this with a dedicated bytecode

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1613: BytecodeLabel end_label;
On 2016/02/03 12:28:26, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1624: BytecodeLabel else_label;
On 2016/02/03 12:28:26, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1636: VariableMode mode =
variable->mode();
On 2016/02/03 12:28:26, rmcilroy wrote:
> DCHECK not legacy const mode

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1641: BuildThrowIfHole(variable->name());
On 2016/02/03 12:28:26, rmcilroy wrote:
> nit - add comment

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1684: break;
On 2016/02/03 12:28:26, rmcilroy wrote:
> nit - add comment on why this break is necessary

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1685: } else if (mode == CONST_LEGACY &&
op != Token::INIT) {
On 2016/02/03 12:28:26, rmcilroy wrote:
> Could you drop this entire branch if you just made mode == CONST_LEGACY && op
!=
> Token::INIT not require a hole check (since the hole check doesn't do anything
> in this case anyway).  You could add the DCHECK(!(mode == CONST_LEGACY && op
!=
> Token::INIT && is_strict(language_mode()))) up above next to the
> hole_check_required boolean if you still want it.

As discussed offline, I am not moving this because we then need a check before
we store to destination at the end of this case.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1687:
builder()->LoadAccumulatorWithRegister(value_temp);
On 2016/02/03 12:28:26, rmcilroy wrote:
> nit - add a comment

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1745: break;
On 2016/02/03 12:28:26, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1747: DCHECK(!is_strict(language_mode()));
On 2016/02/03 12:28:26, rmcilroy wrote:
> Same comment

Same as variable/parameter case earlier.

https://codereview.chromium.org/1634153002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1775: } else if (mode == CONST_LEGACY &&
op != Token::INIT) {
On 2016/02/03 12:28:26, rmcilroy wrote:
> ditto

Same here.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:2635: TEST(JumpOnHole) {
On 2016/02/03 12:28:26, rmcilroy wrote:
> Could you replicate the test-interpreter tests here rather than testing for
just
> JumpOnHole, I think that would be more useful.

Done. Now I have a 1500 line cl :)

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:78: i::FLAG_legacy_const =
true;
On 2016/02/03 12:28:27, rmcilroy wrote:
> You can just set and restore these flags in the test itself rather than adding
a
> new MakeBytecode function.

Done.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:2368: U8(1),                 
               // 58
On 2016/02/03 12:28:27, rmcilroy wrote:
> Fix indenting and remove "58"
> 
> Note: A way of indenting the operands is to use a non-line comment, i.e.:
> B(CallRuntime), U16(Runtime::kThrowReferenceError), R(4),   //
> /*           */ U(1),                                       //
> 

Done. Sorry they were meant to be temporary offsets for my reference, and forgot
to remove them.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:2385: B(Ldar), R(4),         
               // 100
On 2016/02/03 12:28:27, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7589: "const x = 10;",
On 2016/02/03 12:28:26, rmcilroy wrote:
> Fix indentation

Done.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7683: "let x = 10;",
On 2016/02/03 12:28:27, rmcilroy wrote:
> Fix indentation

Done.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:101: i::FLAG_legacy_const = true;
On 2016/02/03 12:28:27, rmcilroy wrote:
> Again you can just do this in the test rather than creating another
GetCallable

Done.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3890: for (size_t i = 0; i <
arraysize(const_decl); i++) {
On 2016/02/03 12:28:27, rmcilroy wrote:
> comment - tests for sloppy mode.

Done.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3928: for (size_t i = 0; i <
arraysize(const_decl); i++) {
On 2016/02/03 12:28:27, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1634153002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:4031: for (size_t i = 0; i <
arraysize(const_decl); i++) {
On 2016/02/03 12:28:27, rmcilroy wrote:
> ditto

Done.

[mythria, 2016-02-04 10:55:28]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 10:55:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/180001

[commit-bot: I haz the power, 2016-02-04 11:09:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-04 11:09:41]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/915)
  v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[mythria, 2016-02-04 11:32:19]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 11:32:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/200001

[commit-bot: I haz the power, 2016-02-04 11:52:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-04 11:52:58]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-02-04 15:19:39]

Looks great, thanks! LGTM.

https://codereview.chromium.org/1634153002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1667: // let x = (x = 20); is not allowed.
/s/let/E.g. let/

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7713: 
nit - please add 
  // clang-format off

and at the end of the snippet add 

  // clang-format on

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7760: U8(0),                 
                          //
/*          */ U8(0)

(and below).

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7805: 
ditto

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7993: // TODO(mythria): Add
tests that walk the context chain.
ditto

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:8115:
ExpectedSnippet<InstanceType> snippets[] = {
ditto

[mythria, 2016-02-04 16:08:00]

Michi, could you please look at changes to bytecode-graph-builder?

https://codereview.chromium.org/1634153002/diff/200001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1667: // let x = (x = 20); is not allowed.
On 2016/02/04 15:19:39, rmcilroy wrote:
> /s/let/E.g. let/

Done.

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7713: 
On 2016/02/04 15:19:39, rmcilroy wrote:
> nit - please add 
>   // clang-format off
> 
> and at the end of the snippet add 
> 
>   // clang-format on

Done.

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7760: U8(0),                 
                          //
On 2016/02/04 15:19:39, rmcilroy wrote:
> /*          */ U8(0)
> 
> (and below).

Done.

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7805: 
On 2016/02/04 15:19:39, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:7993: // TODO(mythria): Add
tests that walk the context chain.
On 2016/02/04 15:19:39, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1634153002/diff/200001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:8115:
ExpectedSnippet<InstanceType> snippets[] = {
On 2016/02/04 15:19:39, rmcilroy wrote:
> ditto

Done.

[mythria, 2016-02-04 17:11:54]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 17:12:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/240001

[commit-bot: I haz the power, 2016-02-04 17:29:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-04 17:29:48]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[rmcilroy, 2016-02-05 16:52:17]

Description was changed from

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interprter.

BUG=v8:4280,v8:4679
LOG=N
==========

to

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interpreter.

BUG=v8:4280,v8:4679
LOG=N
==========

[Michael Starzinger, 2016-02-08 11:54:08]

LGTM with comments to address. Only looked at "compiler", didn't look at the
rest.

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:1522: Node* condition =
NewNode(javascript()->StrictEqual(), accumulator,
Can we use javascript()->StrictNotEqual() here? Or is there a particular reason
that wouldn't work in this case?

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/interpret...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:164: void JumpIfWordNotEqual(Node* lhs,
Node* rhs, Node* jump_offset);
nit: Lets move this up one function to before the "StackCheck" so that all the
jumps are contiguous.

[mythria, 2016-02-08 13:22:33]

Thanks for your review. As explained in my comment, StrictNotEqual does not
work. So I still use StrictEquals. I fixed other comments.

Thanks,
Mythri

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:1522: Node* condition =
NewNode(javascript()->StrictEqual(), accumulator,
On 2016/02/08 11:54:08, Michael Starzinger wrote:
> Can we use javascript()->StrictNotEqual() here? Or is there a particular
reason
> that wouldn't work in this case?

StrictNotEqual causes an assert fail here:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/ic/x64/ic-x...

According to my understanding, parser changes all strictNotEqual to stictEqual
and LogicalNot. So we don't see a strictNotEqual.

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/interpret...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:164: void JumpIfWordNotEqual(Node* lhs,
Node* rhs, Node* jump_offset);
On 2016/02/08 11:54:08, Michael Starzinger wrote:
> nit: Lets move this up one function to before the "StackCheck" so that all the
> jumps are contiguous.

Done.

[mythria, 2016-02-08 13:22:43]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 13:22:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/260001

[commit-bot: I haz the power, 2016-02-08 13:24:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 13:24:47]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/1078)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[mythria, 2016-02-08 13:43:00]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 13:43:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/280001

[Michael Starzinger, 2016-02-08 13:57:45]

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1634153002/diff/240001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:1522: Node* condition =
NewNode(javascript()->StrictEqual(), accumulator,
On 2016/02/08 13:22:33, mythria wrote:
> On 2016/02/08 11:54:08, Michael Starzinger wrote:
> > Can we use javascript()->StrictNotEqual() here? Or is there a particular
> reason
> > that wouldn't work in this case?
> 
> StrictNotEqual causes an assert fail here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/ic/x64/ic-x...
> 
> According to my understanding, parser changes all strictNotEqual to stictEqual
> and LogicalNot. So we don't see a strictNotEqual.

Acknowledged. Ah, yeah, I have seen this before. I am fine with keeping this as
is, maybe I'll follow up on this at some point.

[commit-bot: I haz the power, 2016-02-08 14:03:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 14:03:23]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-02-08 14:10:12]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-02-08 14:10:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1634153002/#ps280001
(title: "Rebased the patch.")

[commit-bot: I haz the power, 2016-02-08 14:10:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634153002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634153002/280001

[commit-bot: I haz the power, 2016-02-08 14:14:43]

Description was changed from

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interpreter.

BUG=v8:4280,v8:4679
LOG=N
==========

to

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interpreter.

BUG=v8:4280,v8:4679
LOG=N
==========

[commit-bot: I haz the power, 2016-02-08 14:14:45]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-02-08 14:15:02]

Description was changed from

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interpreter.

BUG=v8:4280,v8:4679
LOG=N
==========

to

==========
[Interpreter] Adds support for const/let variables to interpreter.

Adds implementation and tests to support const/let variables in the
interpreter.

BUG=v8:4280,v8:4679
LOG=N

Committed: https://crrev.com/90721a51a35644b5dca8116895830b1d10629f68
Cr-Commit-Position: refs/heads/master@{#33819}
==========

[commit-bot: I haz the power, 2016-02-08 14:15:03]

Patchset 15 (id:??) landed as
https://crrev.com/90721a51a35644b5dca8116895830b1d10629f68
Cr-Commit-Position: refs/heads/master@{#33819}