[Interpreter] Minimal bytecode generator.

[Interpreter] Minimal bytecode generator.

Bytecode generator for local assignment and basic binary operations.

Command-line flag for printing bytecodes.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/238397c10866c4b5a02bca00d6a7f0f75cb7fe2d
Cr-Commit-Position: refs/heads/master@{#30221}

[oth, 2015-08-13 16:11:30]

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

[oth, 2015-08-13 16:11:30]

Ross, ptal, this is pretty minimal. Need work out a testing strategy going
forward.

[rmcilroy, 2015-08-14 09:31:18]

Looks great to me (once tests are added). Maybe let Michael have a look too?

For tests, how about adding a cctest (we probably do this as a unittests) which
compiles JS source and checks if the resulting bytecode is as expected?

https://codereview.chromium.org/1294543002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1294543002/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:284: DEFINE_BOOL(print_ignition_bytecode, false, "print
generated bytecode")
nit - just print_bytecode and add Ignition to the description?

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

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:75: // Details stored in scope, ie
variable index.
/s/ie/i.e.

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:202: DCHECK(!expr->IsPropertyName());
Could you make this an if/else or switch statement with the propertyName case
being unimplemented().

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:239: DCHECK(variable->location() ==
VariableLocation::LOCAL);
Could you do this as a switch statement, with all other cases falling through to
undefined for now.

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:268:
builder().StoreAccumulatorInRegister(Register(destination));
We would only be storing the value in a register if it is a VARIABLE case,
wouldn't we? Could you move the switch down so this is part of the VARIABLE case
and destination can be a Register instead of an index?

[oth, 2015-08-17 08:05:48]

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

[oth, 2015-08-17 08:05:49]

Michael, here's the minimal code generator, PTAL.

Ross, comments incorporated. As we discussed, this'll need the the frame
initialization fixes for no register case before committing.

https://codereview.chromium.org/1294543002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1294543002/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:284: DEFINE_BOOL(print_ignition_bytecode, false, "print
generated bytecode")
On 2015/08/14 09:31:17, rmcilroy wrote:
> nit - just print_bytecode and add Ignition to the description?

Done. There's regexp bytecode too, though it only has trace options.

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

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:75: // Details stored in scope, ie
variable index.
On 2015/08/14 09:31:17, rmcilroy wrote:
> /s/ie/i.e.

Done.

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:202: DCHECK(!expr->IsPropertyName());
On 2015/08/14 09:31:17, rmcilroy wrote:
> Could you make this an if/else or switch statement with the propertyName case
> being unimplemented().

Done.

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:239: DCHECK(variable->location() ==
VariableLocation::LOCAL);
On 2015/08/14 09:31:17, rmcilroy wrote:
> Could you do this as a switch statement, with all other cases falling through
to
> undefined for now.

Done.

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:268:
builder().StoreAccumulatorInRegister(Register(destination));
On 2015/08/14 09:31:17, rmcilroy wrote:
> We would only be storing the value in a register if it is a VARIABLE case,
> wouldn't we? Could you move the switch down so this is part of the VARIABLE
case
> and destination can be a Register instead of an index?

Done.

[Michael Starzinger, 2015-08-18 08:17:16]

LGTM from my end. Just nits and suggestions.

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

https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:8: #include
"src/interpreter/bytecode-generator.h"
nit: Please move the include of bytecode-generator.h all the way to the top of
the include list (above "stack" even).

https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:28: DCHECK(scope()->is_function_scope());
I assume this is just a temporary guard, because it won't hold for all
compilation units.

https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:101: stmt->expression()->Accept(this);
nit: Better use Visit(stmt) here. Performs stack-overflow/bailout check and
allows to override BytecodeGenerator::Visit later if needed.

https://codereview.chromium.org/1294543002/diff/20001/src/runtime/runtime-com...
File src/runtime/runtime-compiler.cc (right):

https://codereview.chromium.org/1294543002/diff/20001/src/runtime/runtime-com...
src/runtime/runtime-compiler.cc:39: code->kind() == Code::FUNCTION ||
suggestion: Do you think it makes sense to add a Code::IsJavaScriptCode()
predicate for this? We could also wait until we have more uses of it before we
add it.

https://codereview.chromium.org/1294543002/diff/20001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/1294543002/diff/20001/tools/gyp/v8.gyp#newcod...
tools/gyp/v8.gyp:802: '../../src/interpreter/bytecode-generator.cc',
nit: This will also require BUILD.gn to be adapted.

[rmcilroy, 2015-08-18 10:31:31]

lgtm with a couple of nits.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:23: i::FLAG_print_bytecode =
false;
nit - remove the "i::FLAG_print_bytecode = false"? It should be false by default
and if you don't force it to false then you can pass "--print_bytecode" to the
tests if you want to see the bytecode for a particular test.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:53: 
nit - extra newline

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:112: }}};
indentation seems a little unusual here - is "git cl format" responsable?

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:123: }
nit - extra newline below

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:124: }
nit - comments on closing namespace brackets.

[oth, 2015-08-18 10:56:11]

Thanks both. Feedback incorporated.

Waiting on https://codereview.chromium.org/1289863003/ before committing.

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

https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:8: #include
"src/interpreter/bytecode-generator.h"
On 2015/08/18 08:17:16, Michael Starzinger wrote:
> nit: Please move the include of bytecode-generator.h all the way to the top of
> the include list (above "stack" even).

Done.

https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:28: DCHECK(scope()->is_function_scope());
On 2015/08/18 08:17:16, Michael Starzinger wrote:
> I assume this is just a temporary guard, because it won't hold for all
> compilation units.

Yes, added a comment for other readers.

https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:101: stmt->expression()->Accept(this);
On 2015/08/18 08:17:16, Michael Starzinger wrote:
> nit: Better use Visit(stmt) here. Performs stack-overflow/bailout check and
> allows to override BytecodeGenerator::Visit later if needed.

Done.

https://codereview.chromium.org/1294543002/diff/20001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/1294543002/diff/20001/tools/gyp/v8.gyp#newcod...
tools/gyp/v8.gyp:802: '../../src/interpreter/bytecode-generator.cc',
On 2015/08/18 08:17:16, Michael Starzinger wrote:
> nit: This will also require BUILD.gn to be adapted.

Done.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:23: i::FLAG_print_bytecode =
false;
On 2015/08/18 10:31:31, rmcilroy wrote:
> nit - remove the "i::FLAG_print_bytecode = false"? It should be false by
default
> and if you don't force it to false then you can pass "--print_bytecode" to the
> tests if you want to see the bytecode for a particular test.

Done. It was an explicit clue on debugging problems here.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:53: 
On 2015/08/18 10:31:31, rmcilroy wrote:
> nit - extra newline

Done.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:112: }}};
On 2015/08/18 10:31:31, rmcilroy wrote:
> indentation seems a little unusual here - is "git cl format" responsable?

Yes.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:123: }
On 2015/08/18 10:31:31, rmcilroy wrote:
> nit - extra newline below

Done.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:124: }
On 2015/08/18 10:31:31, rmcilroy wrote:
> nit - comments on closing namespace brackets.

Done.

[oth, 2015-08-18 13:09:58]

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

[commit-bot: I haz the power, 2015-08-18 13:10:01]

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

[commit-bot: I haz the power, 2015-08-18 13:34:31]

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

[commit-bot: I haz the power, 2015-08-18 13:34:31]

Dry run: This issue passed the CQ dry run.

[oth, 2015-08-18 13:45:28]

The CQ bit was checked by oth@chromium.org

[oth, 2015-08-18 13:45:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1294543002/#ps50001
(title: "Incorporate review comments on patch sets 2 & 3.")

[commit-bot: I haz the power, 2015-08-18 13:45:36]

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

[commit-bot: I haz the power, 2015-08-18 13:46:46]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2015-08-18 13:47:02]

Patchset 4 (id:??) landed as
https://crrev.com/238397c10866c4b5a02bca00d6a7f0f75cb7fe2d
Cr-Commit-Position: refs/heads/master@{#30221}