[Intepreter] Extend and move Register class.

[Intepreter] Extend and move Register class.

Add methods for converting parameter index to a register and vice-versa.

Move Register class into bytecodes.h.

Update Bytecodes::Decode to pretty print parameters.

BUG=v8:4280
LOG=NO

Committed: https://crrev.com/22983f71f95140d75faf8d5d0b0b7fdd2624e675
Cr-Commit-Position: refs/heads/master@{#30549}

[oth, 2015-09-02 11:04:13]

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

[oth, 2015-09-02 11:04:14]

rmcilroy@ please take a look. thanks!

[rmcilroy, 2015-09-02 11:42:42]

Looks good. My main suggestion is whether we can move Register class to
bytecodes.h to keep all the logic together a bit more.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:93: class Register {
Would it be simpler just to move the Register class over to bytecodes.h?
(Genuine question)

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

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:123: uint8_t Bytecodes::ParameterIndexToOperand(int
index, int parameter_count) {
Could we make this a Register::FromParameterIndex() instead (and then can use
Register::ToOperand if we need the operand)? This is assuming we can move
Register class over to bytecodes.h as in my earlier comment.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:126: DCHECK_GE(index, 0);
nit - DCHECK_LT(index, parameter_count)

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:144: return 128 + kLastParamRegisterIndex;
This is a bit confusing. Could you use kMinRegisterIndex with the -1 adjustment
(from count to index) and mention that kLastParamRegisterIndex is negative.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:186: os << "this";
nit - could we make it "<this>" or "|this|" or something to make it clear this
is a parameter? Maybe do the same with "<a0>" or "|a0|"as well?

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpre...
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecodes-unittest.cc:24: int parameter_counts[] =
{7, 13, 99};
nit - could you just use a std::vector here instead of having to define
COUNT_OF?

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecodes-unittest.cc:43: for (int i = 0; i < 128;
i++) {
nit - /s/128/kMaxRegisterIndex

[oth, 2015-09-02 14:02:20]

Thanks. Exposed Register in bytecodes.h. I didn't like it when writing PS1/2,
but it is a bit cleaner.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:93: class Register {
On 2015/09/02 11:42:41, rmcilroy wrote:
> Would it be simpler just to move the Register class over to bytecodes.h?
> (Genuine question)

Done.

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

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:123: uint8_t Bytecodes::ParameterIndexToOperand(int
index, int parameter_count) {
On 2015/09/02 11:42:42, rmcilroy wrote:
> Could we make this a Register::FromParameterIndex() instead (and then can use
> Register::ToOperand if we need the operand)? This is assuming we can move
> Register class over to bytecodes.h as in my earlier comment.

Done.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:126: DCHECK_GE(index, 0);
On 2015/09/02 11:42:41, rmcilroy wrote:
> nit - DCHECK_LT(index, parameter_count)

Done.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:144: return 128 + kLastParamRegisterIndex;
On 2015/09/02 11:42:41, rmcilroy wrote:
> This is a bit confusing. Could you use kMinRegisterIndex with the -1
adjustment
> (from count to index) and mention that kLastParamRegisterIndex is negative.

Comment added.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:186: os << "this";
On 2015/09/02 11:42:42, rmcilroy wrote:
> nit - could we make it "<this>" or "|this|" or something to make it clear this
> is a parameter? Maybe do the same with "<a0>" or "|a0|"as well?

Done <this>.

Prefer current consistency of r0, r1, r2, ...a0, a1, etc.

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpre...
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecodes-unittest.cc:24: int parameter_counts[] =
{7, 13, 99};
On 2015/09/02 11:42:42, rmcilroy wrote:
> nit - could you just use a std::vector here instead of having to define
> COUNT_OF?

Done. With uniform initialization, this is a fine idea (std::array even). Chrome
rules seem to be against it, but see it in use elsewhere in v8 tests.

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecodes-unittest.cc:43: for (int i = 0; i < 128;
i++) {
On 2015/09/02 11:42:42, rmcilroy wrote:
> nit - /s/128/kMaxRegisterIndex

Done.

[rmcilroy, 2015-09-02 15:22:29]

Looks much cleaner, thanks. Lgtm once comments are addressed.

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:173: 
nit extra newline

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:182: DCHECK_LE(index, parameter_count);
/s/DCHECK_LE/DCHECK_LT, no?

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:183: DCHECK_LE(parameter_count,
kMaxParameterIndex);
ditto

https://codereview.chromium.org/1325983002/diff/40001/test/unittests/interpre...
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecodes-unittest.cc:27: std::vector<int>
parameter_counts{7, 13, 99};
Unfortunately I don't think this will work on Mac since it is relying on c++11
library features. :(

Could you just do something like:
int parameter_counts[] = {7, 13, 99};
size_t count = sizeof parameter_counts / sizeof(int);
for (size_t p = 0; p < count; p++) {...

[oth, 2015-09-02 16:19:02]

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

[commit-bot: I haz the power, 2015-09-02 16:19:08]

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

[oth, 2015-09-02 16:26:08]

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

[commit-bot: I haz the power, 2015-09-02 16:26:19]

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

[oth, 2015-09-02 16:28:06]

Thanks.

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:173: 
On 2015/09/02 15:22:29, rmcilroy wrote:
> nit extra newline

Done.

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:182: DCHECK_LE(index, parameter_count);
On 2015/09/02 15:22:29, rmcilroy wrote:
> /s/DCHECK_LE/DCHECK_LT, no?

Done.

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:183: DCHECK_LE(parameter_count,
kMaxParameterIndex);
On 2015/09/02 15:22:29, rmcilroy wrote:
> ditto

Parameter count can be 1 larger than kMaxParameterIndex, no?

DCHECK_LE(parameter_count, kMaxParameterIndex + 1).

https://codereview.chromium.org/1325983002/diff/40001/test/unittests/interpre...
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/test/unittests/interpre...
test/unittests/interpreter/bytecodes-unittest.cc:27: std::vector<int>
parameter_counts{7, 13, 99};
On 2015/09/02 15:22:29, rmcilroy wrote:
> Unfortunately I don't think this will work on Mac since it is relying on c++11
> library features. :(
> 
> Could you just do something like:
> int parameter_counts[] = {7, 13, 99};
> size_t count = sizeof parameter_counts / sizeof(int);
> for (size_t p = 0; p < count; p++) {...

Done, but with sizeof(parameter_counts[0]) to preserve type.

[commit-bot: I haz the power, 2015-09-02 16:29:29]

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

[commit-bot: I haz the power, 2015-09-02 16:29:30]

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

[oth, 2015-09-02 16:49:40]

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

[commit-bot: I haz the power, 2015-09-02 16:49:47]

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

[rmcilroy, 2015-09-02 17:08:53]

Still lgtm, thanks!

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.cc:183: DCHECK_LE(parameter_count,
kMaxParameterIndex);
Yes your right, thanks.

[commit-bot: I haz the power, 2015-09-02 17:16:09]

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

[commit-bot: I haz the power, 2015-09-02 17:16:10]

Dry run: This issue passed the CQ dry run.

[oth, 2015-09-02 17:46:26]

The CQ bit was checked by oth@chromium.org

[commit-bot: I haz the power, 2015-09-02 17:46:36]

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

[commit-bot: I haz the power, 2015-09-02 17:48:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-02 17:48:33]

Patchset 5 (id:??) landed as
https://crrev.com/22983f71f95140d75faf8d5d0b0b7fdd2624e675
Cr-Commit-Position: refs/heads/master@{#30549}