[Interpreter] Enable assignments in expressions.

[Interpreter] Enable assignments in expressions.

This change introduces register re-mapping to avoid assignment hazards
in binary expressions. Expressions that cause problems typically have
the form y = x + (x = 4);. The problem occurs because the lhs value
evaluates to the register holding x. The rhs updates that register and
then applying the operation would use the new value as the lhs.

By tracking loads and stores in binary expressions the generator is now
able to detect when condition occurs and uses a temporary register for
the rhs value. When the binary expression evaluation is complete the
variable is updated with the latest temporary.

A new bytecode Mov performs this update without touching the
accumulator.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/a1ba971cd84a77d077f5fb3389c3f6c66392cdae
Cr-Commit-Position: refs/heads/master@{#32141}

[oth, 2015-11-02 12:30:28]

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

[oth, 2015-11-02 12:30:28]

Ross, PTAL. I'll circulate more widely when you've had a look over. Thanks!

[rmcilroy, 2015-11-03 14:17:52]

Overall looks good, just some minor comments.

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

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:303: class
BytecodeGenerator::StatementResultScope : public EffectResultScope {
nit - final

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:320: Register
LoadFromAliasedRegister(Register reg) {
Could you add a comment explaining what the return value is, also mark as
MUST_USE

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:324: auto iterator = insert_result.first;
nit - /s/iterator/previous_value

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:340: }
nit - a few comments here would be helpful

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:354: void StopAliasingRegisters() {
nit - RestoreAliasedLocalsAndParameters?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1280: if (statement_execution_result()) {
Should we not always have a statement_execution_result scope when visiting
variable loads?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1281: source =
statement_execution_result()->LoadFromAliasedRegister(source);
nit - RecordLoadFrom...

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1356: if (statement_execution_result()) {
Ditto with assignments?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1358:
statement_execution_result()->StoreToAliasedRegister(destination);
nit - RecordStoreTo...

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1939:
statement_execution_result()->EnterAssignmentHazard();
Could we do this as a scoped class (AssignmentHazardScope?) instead, just to
ensure we always correctly leave the assignment hazard area even if we return in
the middle of a visit function?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:59: void VisitCondition(Expression* expr);
Should this be VisitForCondition or VisitForTest (to be consistent with
AstGraphBuilder) and put down next to VisitFor... operators?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:87: void
VisitExpressionStatementExpression(Expression* expr);
Unused?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (left):

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:274: void operator delete(void* p);
We could keep this by using index() in the containers instead, but I'll leave it
to you.

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

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:266: bool operator<(const Register& other) const {
return index_ < other.index_; }
Could you check why this is needed?

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

https://codereview.chromium.org/1412683011/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:4998: // TODO(oth): Add
return x + (x++) + (++x);
Add this?

[rmcilroy, 2015-11-03 14:17:53]

Overall looks good, just some minor comments.

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

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:303: class
BytecodeGenerator::StatementResultScope : public EffectResultScope {
nit - final

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:320: Register
LoadFromAliasedRegister(Register reg) {
Could you add a comment explaining what the return value is, also mark as
MUST_USE

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:324: auto iterator = insert_result.first;
nit - /s/iterator/previous_value

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:340: }
nit - a few comments here would be helpful

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:354: void StopAliasingRegisters() {
nit - RestoreAliasedLocalsAndParameters?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1280: if (statement_execution_result()) {
Should we not always have a statement_execution_result scope when visiting
variable loads?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1281: source =
statement_execution_result()->LoadFromAliasedRegister(source);
nit - RecordLoadFrom...

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1356: if (statement_execution_result()) {
Ditto with assignments?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1358:
statement_execution_result()->StoreToAliasedRegister(destination);
nit - RecordStoreTo...

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1939:
statement_execution_result()->EnterAssignmentHazard();
Could we do this as a scoped class (AssignmentHazardScope?) instead, just to
ensure we always correctly leave the assignment hazard area even if we return in
the middle of a visit function?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:59: void VisitCondition(Expression* expr);
Should this be VisitForCondition or VisitForTest (to be consistent with
AstGraphBuilder) and put down next to VisitFor... operators?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:87: void
VisitExpressionStatementExpression(Expression* expr);
Unused?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (left):

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:274: void operator delete(void* p);
We could keep this by using index() in the containers instead, but I'll leave it
to you.

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

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:266: bool operator<(const Register& other) const {
return index_ < other.index_; }
Could you check why this is needed?

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

https://codereview.chromium.org/1412683011/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:4998: // TODO(oth): Add
return x + (x++) + (++x);
Add this?

[oth, 2015-11-04 10:03:37]

PTAL. The code now uses AssignmentHazardScopes. It drops StatementResultScope
entirely, but has EffectResultScope where these used to be instantiated to
provide the outer scope needed in various places. WDYT?

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

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:303: class
BytecodeGenerator::StatementResultScope : public EffectResultScope {
On 2015/11/03 14:17:52, rmcilroy wrote:
> nit - final

Done.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:320: Register
LoadFromAliasedRegister(Register reg) {
On 2015/11/03 14:17:52, rmcilroy wrote:
> Could you add a comment explaining what the return value is, also mark as
> MUST_USE

Done.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:324: auto iterator = insert_result.first;
On 2015/11/03 14:17:52, rmcilroy wrote:
> nit - /s/iterator/previous_value

Renamed to 'mapping' as it's either the value just inserted or the existing
value if it was already present.

Added a comment here.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:340: }
On 2015/11/03 14:17:52, rmcilroy wrote:
> nit - a few comments here would be helpful

Done.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:354: void StopAliasingRegisters() {
On 2015/11/03 14:17:52, rmcilroy wrote:
> nit - RestoreAliasedLocalsAndParameters?

Done. Definitely a better name.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1280: if (statement_execution_result()) {
On 2015/11/03 14:17:51, rmcilroy wrote:
> Should we not always have a statement_execution_result scope when visiting
> variable loads?

Right, this bit is tricky. There are places we reach here from that are not
trivial to resolve, e.g. Visiting switch statements for tag and case values. If
the StatementResultScope goes at the top-level there then return statements in
switch statements end up creating a nested StatementResultScope.

As discussed suggest we split HazardAssignmentScope into a separate class that
is instantiated in the VisitArithmeticOperation and VisitCompareOperation
methods and deals with the hazard assignment logic. The code keeps
StatementResultScope but just uses it as the outer allocation scope for places
where visits allocate registers in the parent scope.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1281: source =
statement_execution_result()->LoadFromAliasedRegister(source);
On 2015/11/03 14:17:51, rmcilroy wrote:
> nit - RecordLoadFrom...

GetRegisterToLoadFrom?
GetRegisterToStoreTo?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1356: if (statement_execution_result()) {
On 2015/11/03 14:17:51, rmcilroy wrote:
> Ditto with assignments?

Changed to assignment_hazard_scope, but have to check  if it exists or not
still.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1358:
statement_execution_result()->StoreToAliasedRegister(destination);
On 2015/11/03 14:17:52, rmcilroy wrote:
> nit - RecordStoreTo...

GetRegisterForStore okay?

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1939:
statement_execution_result()->EnterAssignmentHazard();
On 2015/11/03 14:17:52, rmcilroy wrote:
> Could we do this as a scoped class (AssignmentHazardScope?) instead, just to
> ensure we always correctly leave the assignment hazard area even if we return
in
> the middle of a visit function?

Done.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:59: void VisitCondition(Expression* expr);
On 2015/11/03 14:17:52, rmcilroy wrote:
> Should this be VisitForCondition or VisitForTest (to be consistent with
> AstGraphBuilder) and put down next to VisitFor... operators?

Removed from latest revision as no longer need to worry about these.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:87: void
VisitExpressionStatementExpression(Expression* expr);
On 2015/11/03 14:17:52, rmcilroy wrote:
> Unused?

Done.

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (left):

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:274: void operator delete(void* p);
On 2015/11/03 14:17:52, rmcilroy wrote:
> We could keep this by using index() in the containers instead, but I'll leave
it
> to you.

Put these back and went with index() in the containers.

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

https://codereview.chromium.org/1412683011/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:266: bool operator<(const Register& other) const {
return index_ < other.index_; }
On 2015/11/03 14:17:52, rmcilroy wrote:
> Could you check why this is needed?

Restored. No longer needed, not sure where in the history this was needed.

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

https://codereview.chromium.org/1412683011/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:4998: // TODO(oth): Add
return x + (x++) + (++x);
On 2015/11/03 14:17:52, rmcilroy wrote:
> Add this?

Done.

[rmcilroy, 2015-11-05 16:34:51]

A couple of suggestions, but lgtm.

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

https://codereview.chromium.org/1412683011/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:241: class
BytecodeGenerator::EffectResultScope : public ExpressionResultScope {
Make final again

https://codereview.chromium.org/1412683011/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:311:
generator->set_assignment_hazard_scope(this);
Rather than creating a map and set for each AssignmentHazardScope, could
AssignmentHazardScope just be a wrapper around another class which does the
actual register remapping, and create / set that class if the
AssignmentHazardScope is the outer-most one. E.g.:

explicit AssignmentHazardScope(BytecodeGenerator* generator) {
  if (generator->aliased_local_and_parameter_helper() == nullptr) {
    helper_ = new AliasedLocalAndParameterHelper(generator);
    generator->set_aliased_local_and_parameter_helper(helper_);
  }
}

~AssignmentHazardScope() {
  if (helper_) {
    helper_->RestoreAliasedLocalsAndParameters();
    generator->set_aliased_local_and_parameter_helper(nullptr);
  }
}

And move the rest of the code into AliasedLocalAndParameterHelper, WDYT?

Actually, thinking about it further, the AliasedLocalAndParameterHelper could
just be a permanent member of generator and AssignmentHazardScope could just
call EnableAssignmentHazard() and RestoreAliasedLocalsAndParameters() in the
constructor / destructor, which would avoid the "if (assignment_hazard_scope())
{" checks in  VisitVariableLoad / VisitVariableAssignment).

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

https://codereview.chromium.org/1412683011/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:59: void VisitCondition(Expression* expr);
Remove - no longer used.

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

https://codereview.chromium.org/1412683011/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3002: 5},
Could you add a test for something like: 

function f(a) { return a + (arguments[0] = 10) }

as suggested by Toon in https://codereview.chromium.org/1392933002/.

[oth, 2015-11-12 11:32:15]

Hi Ross, if you've not gone on leave yet PTAL. Thanks!

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

https://codereview.chromium.org/1412683011/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:241: class
BytecodeGenerator::EffectResultScope : public ExpressionResultScope {
On 2015/11/05 16:34:51, rmcilroy wrote:
> Make final again

Done.

https://codereview.chromium.org/1412683011/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:311:
generator->set_assignment_hazard_scope(this);
On 2015/11/05 16:34:51, rmcilroy wrote:
> Rather than creating a map and set for each AssignmentHazardScope, could
> AssignmentHazardScope just be a wrapper around another class which does the
> actual register remapping, and create / set that class if the
> AssignmentHazardScope is the outer-most one. E.g.:
> 
> explicit AssignmentHazardScope(BytecodeGenerator* generator) {
>   if (generator->aliased_local_and_parameter_helper() == nullptr) {
>     helper_ = new AliasedLocalAndParameterHelper(generator);
>     generator->set_aliased_local_and_parameter_helper(helper_);
>   }
> }
> 
> ~AssignmentHazardScope() {
>   if (helper_) {
>     helper_->RestoreAliasedLocalsAndParameters();
>     generator->set_aliased_local_and_parameter_helper(nullptr);
>   }
> }
> 
> And move the rest of the code into AliasedLocalAndParameterHelper, WDYT?
> 
> Actually, thinking about it further, the AliasedLocalAndParameterHelper could
> just be a permanent member of generator and AssignmentHazardScope could just
> call EnableAssignmentHazard() and RestoreAliasedLocalsAndParameters() in the
> constructor / destructor, which would avoid the "if
(assignment_hazard_scope())
> {" checks in  VisitVariableLoad / VisitVariableAssignment).

Okay, the helper class an inner class of BytecodeGenerator. The state was in
BytecodeGenerator before the detour into StatementExpessionResult. The test
in load/stores happens in the GetRegisterForLoad/Store so it looks cleaner.

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

https://codereview.chromium.org/1412683011/diff/120001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3002: 5},
On 2015/11/05 16:34:51, rmcilroy wrote:
> Could you add a test for something like: 
> 
> function f(a) { return a + (arguments[0] = 10) }
> 
> as suggested by Toon in https://codereview.chromium.org/1392933002/.

Done.

[rmcilroy, 2015-11-12 12:49:15]

Looks great, a couple of DCHECK suggestions but LGTM!

https://codereview.chromium.org/1412683011/diff/180001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1412683011/diff/180001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:1: /// Copyright 2015 the V8 project
authors. All rights reserved.
remove extra '/'

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

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:316: void
BytecodeGenerator::AssignmentHazardHelper::LeaveScope() {
DCHECK_GT(scope_depth, 0)

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:318: RestoreAliasedLocalsAndParameters();
DCHECK_EQ(execution_result_, generator_->execution_result(); ?

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:52: void LeaveScope();
nit - move Enter / Leave to private and make AssignmentHazardScope a friend
class?

[oth, 2015-11-13 12:43:30]

Thanks!

https://codereview.chromium.org/1412683011/diff/180001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1412683011/diff/180001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:1: /// Copyright 2015 the V8 project
authors. All rights reserved.
On 2015/11/12 12:49:15, rmcilroy wrote:
> remove extra '/'

Done.

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

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:316: void
BytecodeGenerator::AssignmentHazardHelper::LeaveScope() {
On 2015/11/12 12:49:15, rmcilroy wrote:
> DCHECK_GT(scope_depth, 0)

Done.

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:318: RestoreAliasedLocalsAndParameters();
On 2015/11/12 12:49:15, rmcilroy wrote:
> DCHECK_EQ(execution_result_, generator_->execution_result(); ? 

Good idea. Done.

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1412683011/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-generator.h:52: void LeaveScope();
On 2015/11/12 12:49:15, rmcilroy wrote:
> nit - move Enter / Leave to private and make AssignmentHazardScope a friend
> class?

Done.

[oth, 2015-11-13 13:27:03]

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

[oth, 2015-11-13 13:27:04]

mstarzinger@chromium.org: Please review changes in compiler and anywhere else of
interest. This is hopefully the last of the craziness / complexity in the
bytecode generator.

[Michael Starzinger, 2015-11-20 09:45:15]

LGTM on "compiler". The changes to BytecodeGenerator scare, confuse and frighten
me, so I cannot comment on them.

[oth, 2015-11-20 10:53:38]

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

[commit-bot: I haz the power, 2015-11-20 10:53:43]

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

[commit-bot: I haz the power, 2015-11-20 11:11:03]

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

[commit-bot: I haz the power, 2015-11-20 11:11:03]

Dry run: This issue passed the CQ dry run.

[oth, 2015-11-20 11:15:46]

The CQ bit was checked by oth@chromium.org

[oth, 2015-11-20 11:15:46]

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/1412683011/#ps260001
(title: "Sync test bytecode sequences to avoid unnecessary Ldar/Star
instructions.")

[commit-bot: I haz the power, 2015-11-20 11:15:53]

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

[commit-bot: I haz the power, 2015-11-20 11:17:16]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-20 11:18:00]

Patchset 14 (id:??) landed as
https://crrev.com/a1ba971cd84a77d077f5fb3389c3f6c66392cdae
Cr-Commit-Position: refs/heads/master@{#32141}