[interpreter] Add accumulator use description to bytecodes.

[interpreter] Add accumulator use description to bytecodes.

Anotates bytecodes with a description of how each uses the accumulator.

Validates annotations and uses of accumulator when generating bytecode
handlers.

Only prints the accumulator during tracing where used.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/7f3442133e5fb34827fe05cf12e2d89c638cc606
Cr-Commit-Position: refs/heads/master@{#35281}

[oth, 2016-04-04 09:27:56]

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

[oth, 2016-04-04 09:27:56]

Ross, PTAL, thanks.

[rmcilroy, 2016-04-05 12:59:23]

LGTM

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:253: kWrite = 2,
Nit - could you make this 1 << 1 (and read 1 << 0) to make it clear these are
used as bitmasks.

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:50: CHECK_EQ(accumulator_use_,
Bytecodes::GetAccumulatorUse(bytecode_));
DCHECK_EQ - also please add a comment on how to fix if this fails.

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.h:157: // Returns the accumulator value
without checking whether bytecode uses it.
nit - mention it should only be used for passing accumulator value on dispatch
or for tracing. Also, I'd prefer if this was in a block of it's own (below these
4 methods).

https://codereview.chromium.org/1852213002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1852213002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecodes-unittest.cc:315: CHECK_EQ(names.size(), 4);
Not sure what this is testing?

[oth, 2016-04-05 14:12:10]

Thanks, comments incorporated.

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:253: kWrite = 2,
On 2016/04/05 12:59:22, rmcilroy (Slow) wrote:
> Nit - could you make this 1 << 1 (and read 1 << 0) to make it clear these are
> used as bitmasks.

Done.

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:50: CHECK_EQ(accumulator_use_,
Bytecodes::GetAccumulatorUse(bytecode_));
On 2016/04/05 12:59:22, rmcilroy (Slow) wrote:
> DCHECK_EQ - also please add a comment on how to fix if this fails.

Comment added and switched to DCHECK though I disagree with the choice.

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1852213002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.h:157: // Returns the accumulator value
without checking whether bytecode uses it.
On 2016/04/05 12:59:23, rmcilroy (Slow) wrote:
> nit - mention it should only be used for passing accumulator value on dispatch
> or for tracing. Also, I'd prefer if this was in a block of it's own (below
these
> 4 methods).

Done.

https://codereview.chromium.org/1852213002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1852213002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/bytecodes-unittest.cc:315: CHECK_EQ(names.size(), 4);
On 2016/04/05 12:59:23, rmcilroy (Slow) wrote:
> Not sure what this is testing? 

It tests the associated names of the AccumulatorUse enumeration are unique. It
avoids AccumulatorUseToString showing up as untouched by the code coverage
tooling so is one less thing anyone has to glance at.

[oth, 2016-04-06 07:34:07]

The CQ bit was checked by oth@chromium.org

[oth, 2016-04-06 07:34:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1852213002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-04-06 07:34:16]

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

[commit-bot: I haz the power, 2016-04-06 07:57:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-06 07:58:05]

Description was changed from

==========
[interpreter] Add accumulator use description to bytecodes.

Anotates bytecodes with a description of how each uses the accumulator.

Validates annotations and uses of accumulator when generating bytecode
handlers.

Only prints the accumulator during tracing where used.

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

to

==========
[interpreter] Add accumulator use description to bytecodes.

Anotates bytecodes with a description of how each uses the accumulator.

Validates annotations and uses of accumulator when generating bytecode
handlers.

Only prints the accumulator during tracing where used.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/7f3442133e5fb34827fe05cf12e2d89c638cc606
Cr-Commit-Position: refs/heads/master@{#35281}
==========

[commit-bot: I haz the power, 2016-04-06 07:58:06]

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