[Interpreter] Removes assignment hazard scope.

[Interpreter] Removes assignment hazard scope.

Removes assignment hazard scope. Reverts back to the naive scheme of
allocating a temporary for every variable load. It was decided to revert it
because the current implementation does not handle logical expressions,
ternary operators, visiting objects in named/keyed loads. Also, we wanted
to evaluate alternate approaches and choose one when we have a mechanism
to measure performance.

BUG=v8:4280
LOG=N

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

[mythria, 2016-01-12 14:33:11]

mythria@chromium.org changed reviewers:
+ oth@chromium.org, rmcilroy@google.com

[mythria, 2016-01-12 14:33:12]

PTAL,

Thanks,
Mythri

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

https://codereview.chromium.org/1576403004/diff/20001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:815: 'compiler/regress-variable-liveness': [SKIP],
This fails because of deopting an inlined function.

https://codereview.chromium.org/1576403004/diff/20001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:1108: 'setter-on-constructor-prototype': [SKIP],
I have a fix for this as a different cl. This fails because when visiting object
literal, the literal register allocated in outer scope is not contiguous with
the other registers allocated in the inner scope.

[rmcilroy, 2016-01-12 14:43:17]

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

[rmcilroy, 2016-01-12 14:43:17]

LGTM, thanks for doing this!

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

https://codereview.chromium.org/1576403004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:223: // implementation. Remove this if not
required.
I'd rather just remove this now if it is no longer needed (perhaps adding a TODO
that it might be necessary to allocated a new register here for outer scopes. If
you remove this call, also remove AllocateNewRegister since this is the only
place it's called.

https://codereview.chromium.org/1576403004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:601: EffectResultScope
switch_result_scope(this);
nit - effect_results_scope

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

https://codereview.chromium.org/1576403004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:296: 2*kPointerSize,
Space between '*' and lhs / rhs (throughout)

[mythria, 2016-01-12 16:53:21]

Thanks, I fixed them.

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

https://codereview.chromium.org/1576403004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:223: // implementation. Remove this if not
required.
On 2016/01/12 14:43:17, rmcilroy wrote:
> I'd rather just remove this now if it is no longer needed (perhaps adding a
TODO
> that it might be necessary to allocated a new register here for outer scopes.
If
> you remove this call, also remove AllocateNewRegister since this is the only
> place it's called.

Done.

https://codereview.chromium.org/1576403004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:601: EffectResultScope
switch_result_scope(this);
On 2016/01/12 14:43:17, rmcilroy wrote:
> nit - effect_results_scope

Done.

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

https://codereview.chromium.org/1576403004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:296: 2*kPointerSize,
On 2016/01/12 14:43:17, rmcilroy wrote:
> Space between '*' and lhs / rhs (throughout)

Done.

[rmcilroy, 2016-01-12 16:59:08]

still lgtm, thanks!

[oth, 2016-01-13 08:57:51]

lgtm. Thanks for taking this on, not fun, but high value.

[mythria, 2016-01-13 14:52:06]

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

[commit-bot: I haz the power, 2016-01-13 14:52:13]

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

[commit-bot: I haz the power, 2016-01-13 15:34:54]

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

[commit-bot: I haz the power, 2016-01-13 15:34:54]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-01-13 15:38:52]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-01-13 15:38:53]

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

[commit-bot: I haz the power, 2016-01-13 15:39:04]

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

[commit-bot: I haz the power, 2016-01-13 15:40:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-13 15:41:39]

Description was changed from

==========
[Interpreter] Removes assignment hazard scope.

Removes assignment hazard scope. Reverts back to the naive scheme of
allocating a temporary for every variable load. It was decided to revert it
because the current implementation does not handle logical expressions,
ternary operators, visiting objects in named/keyed loads. Also, we wanted
to evaluate alternate approaches and choose one when we have a mechanism
to measure performance.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Removes assignment hazard scope.

Removes assignment hazard scope. Reverts back to the naive scheme of
allocating a temporary for every variable load. It was decided to revert it
because the current implementation does not handle logical expressions,
ternary operators, visiting objects in named/keyed loads. Also, we wanted
to evaluate alternate approaches and choose one when we have a mechanism
to measure performance.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/eccbdde0419c420cea3f739d6aaab1e69b6f1343
Cr-Commit-Position: refs/heads/master@{#33269}
==========

[commit-bot: I haz the power, 2016-01-13 15:41:40]

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