[interpreter] Wide register support.

[interpreter] Wide register support.

This increases the size of register operands to be 16-bit.

Not all bytecodes have wide register variants, so when they are
needed a register translator will copy them into a small area
reserved at the top of the 8-bit register range and these registers
are supplied as arguments to the bytecode with 8-bit operands.

This is non-intrusive for typical bytecode where the number of
registers is less than 120. For bytecodes with wide register
operands (above the window) their index needs to be translated
to avoid the reserved translation window.

Enables splay.js to run in Octane and a handful of mjsunit tests.

BUG=v8:4280, v8:4675
LOG=NO

Committed: https://crrev.com/19df7a20f0d50e3b6a8db39c39ee1cd157c56d25
Cr-Commit-Position: refs/heads/master@{#33516}

[oth, 2016-01-21 14:59:19]

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

[oth, 2016-01-21 14:59:20]

This is a snapshot of the register translation. PTAL to see the approach is
agreeable.

More tests are in progress, if there are specific tests you think of, let me
know.

- Propose in a follow-on CL to move the temporary register allocator into it's
own file and make it independently testable.

- Rename Register::ToRawOperand/ToOperand/ToWideOperand to
ToOperand/ToReg8Operand/ToReg16Operand and ditto FromOperand.

Thanks!

[rmcilroy, 2016-01-22 17:50:57]

Overall looks great. I've not looked at the tests yet. A couple of suggestions.

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:75:
builder->set_register_translation_scope(this);
DCHECK_EQ(nullptr, builder()->register_translation_scope());

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:284: Output(translator.bytecode(),
reg.ToRawOperand());
To avoid the extra translate call, could we write this as:

  RegisterTranslationScope translator(this, BytecodeForBinaryOperation(op));
  Output(translator.bytecode(), translator.ToRawOperand(reg));

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:417: RegisterTranslationScope
translator(this);
Personally I would prefer that the RegisterTranslationScope always takes a
bytecode in the constructor, and then you have a bytecode variable here which is
set based on the operand sizes (i.e., move the RegisterTranslationScope below
this if/else. Would that work? e.g.:
 
   Bytecode bytecode;
   if (FitsInReg8Operand(from) && FitsInReg8Operand(to)) {
     bytecode = Bytecode::kMov
   } else ....
     ...
   }
   RegisterTranslationScope translator(this, bytecode);
   Output(translator.bytecode(), translator.ToRawOperand(from),
translator.ToRawOperand(to));

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:434: if (!from.is_valid() ||
!to.is_valid()) {
Just DCHECK(from.is_valid() && to.is_valid)) ?

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:579:
static_cast<uint16_t>(feedback_slot));
Maybe just cast directly to uint32_t now so that we don't accidentally down-cast
a 32bit value if we add 32bit operands (and given we do the checks in
OperandIsValid on size.

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1361: if (free_temporaries_.empty()) {
Please add a comment on why you are having to forge a new temporary reg here.

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1380: // a hidden discontinuity.
/s/discontinuity/hole in allocatable registers ?

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1523: translation_register_count());
Does this not check for the temp being live. Could you do this with checking
either TemporaryRegisterIsLive(reg) ||
RegisterTranslator::InTranslationWindow(reg) ?

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1539: Register previous =
nit - previous_reg

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

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:68: // Returns the number of used for
translating wide register operands
/s/of used/of registers used

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:69: // into byte sized register
operands..
remove extra full-stop.

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

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:328: static Register IllegalRegister() { return
Register(kIllegalIndex); }
This is just the same as Register(); I'm fine with keeping this if if you think
it's clearer though (although maybe make it illegal_register() to be in line
with things like function_closure() etc.

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
File src/interpreter/register-translator.cc (right):

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.cc:21: DCHECK_EQ(bytecode, bytecode_);
I'm not sure what the point of passing bytecode to this function is? is it just
for this check? Seems unnecessary since the RegisterTranslator is explicitly
created in the same function as is outputting the bytecode.

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.cc:28:
register_operands_[register_operands_count_] = addressable.ToRawOperand();
Do we need to keep the register_operands_ in vector? It only seems to be used
for a DCHECK, and I think that if we do the pattern above where the
translator.ToRawOperand(reg) happens in the arguments to the output then there
is less need to make sure that we pass translated arguments (e.g., make the
check that register_operands_count_ ==
Bytecodes::NumberOfRegisterOperands(bytecode).

If you really want to keep this, then move it behind a DEBUG #ifdef.

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.cc:62: // Only tests should visit this path.
What test kick this path? It seems non-obvious that this function won't actually
do a move unless the operand type is kReg8. Maybe just add a check in Translate
that does:
  if (addressable != translated_reg && to_operand_type == kReg8) {
    mover()->MoveRegisterUntranslated(from, to);
  }
- or just add a DCHECK that to_operand_type is kReg8 in this case?

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.cc:107: next_operand_type ==
OperandType::kRegCount16;
You don't seem to use next_operand_is_reg_count ?

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.cc:199: return Register(reg.index() +
kTranslationWindowLength);
I wonder whether we should have a TranslatedRegister type just to differentiate
them and ensure we don't accidentally use the wrong type. Not sure how invasive
this would be (feel free to make it a TODO for another CL if you agree but it
would be invasive). WDYT?

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.cc:210: for (int operand = 0; operand <
operand_count; ++operand) {
Would it be possible to calculate this statically using bytecode traits? As an
alternative, maybe you could keep track of the current operand_index (and update
it in this loop) so that you continue from the previous operand index point
rather than starting from the start each time?

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
File src/interpreter/register-translator.h (right):

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.h:25: };
nit - just declare RegisterMover here and move the definition below
RegisterTranslator (which is the main class for this file).

https://codereview.chromium.org/1613163002/diff/40001/src/interpreter/registe...
src/interpreter/register-translator.h:41: // This class does not support moving
ranges of registers to and fro
/s/fro/from

[rmcilroy, 2016-01-22 17:50:57]

Overall looks great. I've not looked at the tests yet. A couple of suggestions.

[oth, 2016-01-24 10:19:00]

Thanks for the feedback.

I was doubtful of the approach in this CL and the feedback convinced me of this
even though it didn't call out to do something different. The code has gone back
to first path of non-intrusive changes to the BytecodeArrayBuilder and the
original pain points have been resolved.

Can you PTAL again? Thanks.

[rmcilroy, 2016-01-25 12:12:05]

Looks great, just a few comments.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:203:
register_translator()->UntranslateRegisters();
Not sure about the naming of UntranslateRegisters. How about TranslateRegisters
-> TranslateAndMoveRegisters()  and UntranslateRegisters ->
RestoreTranslatedRegisters() ?

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:380: // Move bytecodes modify the
stack. Chacking validity is essential.
I'm not sure what this comment means? I'm guessing this is explaining why
RegisterIsValid is here with CHECK instead of DCHECK. Star also modifies the
stack, should we add the check there too, or do it for all kReg8Output registers
when we add that operand. 

Either way, /s/Chacking/Checking

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1442: return reg.index() <
register_limit;
Same comment as last time on this line - looks like we have less checking here
since we are just checking the register is in the valid range and not whether it
is live. Is this intended?

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecodes.h:395: // Returns a zero-based bit of the register
operand positions of |bitmap|.
/s/bit/bitmap/ and /s/|bitmap|/|bytecode|

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
File src/interpreter/register-translator.cc (right):

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:57:
mover()->MoveRegisterUntranslated(invalid_reg, invalid_reg);
Maybe we can make this a Death test instead? 

If we can't do a death test then personally I would prefer that there was a
"test mode" on this class and we CHECK failed if this was false, and did
something exposable to tests if it was true, rather than relying on the layer
above having a CHECK for invalid_reg. Whatever the decision, please make it
explicit with a comment here.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:103: operand_type ==
OperandType::kRegCount16) {
Should this not be checked in OperandIsValid in bytecode array builder instead
of here? It seems a bit strange to split this up between here and bytecode array
builder

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:133: bool
RegisterTranslator::OperandIsMoveable(Bytecode bytecode,
This could be exposed as part of the RegisterMover delegate to keep the logic of
which bytecodes are moveable and whether a reg operand followed by a regcount
operand means a reg range in there.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:199: // TODO(oth): Move translation
window in this scenario.
Explain why (i.e., the translation window causes a large frame size).

[oth, 2016-01-25 16:51:57]

Thanks, all incorporated.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:203:
register_translator()->UntranslateRegisters();
On 2016/01/25 12:12:05, rmcilroy wrote:
> Not sure about the naming of UntranslateRegisters. How about
TranslateRegisters
> -> TranslateAndMoveRegisters()  and UntranslateRegisters ->
> RestoreTranslatedRegisters() ?

Done as discussed TranslateInputRegisters/TranslateOutputRegisters to match
follow-on CL.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:380: // Move bytecodes modify the
stack. Chacking validity is essential.
On 2016/01/25 12:12:05, rmcilroy wrote:
> I'm not sure what this comment means? I'm guessing this is explaining why
> RegisterIsValid is here with CHECK instead of DCHECK. Star also modifies the
> stack, should we add the check there too, or do it for all kReg8Output
registers
> when we add that operand. 
> 
> Either way, /s/Chacking/Checking 

Have changed this to a DCHECK for the purpose of this CL. I tend to think all
register operands should be checked, but not in this CL. The particular one to
watch at this juncture is in MoveRegisterUntranslated which remains a CHECK.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1442: return reg.index() <
register_limit;
On 2016/01/25 12:12:05, rmcilroy wrote:
> Same comment as last time on this line - looks like we have less checking here
> since we are just checking the register is in the valid range and not whether
it
> is live. Is this intended?

No, brain melt. Fixed.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/byteco...
src/interpreter/bytecodes.h:395: // Returns a zero-based bit of the register
operand positions of |bitmap|.
On 2016/01/25 12:12:05, rmcilroy wrote:
> /s/bit/bitmap/ and /s/|bitmap|/|bytecode|

Done.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
File src/interpreter/register-translator.cc (right):

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:57:
mover()->MoveRegisterUntranslated(invalid_reg, invalid_reg);
On 2016/01/25 12:12:05, rmcilroy wrote:
> Maybe we can make this a Death test instead? 
> 
> If we can't do a death test then personally I would prefer that there was a
> "test mode" on this class and we CHECK failed if this was false, and did
> something exposable to tests if it was true, rather than relying on the layer
> above having a CHECK for invalid_reg. Whatever the decision, please make it
> explicit with a comment here.

Didn't appreciate death test was an option. Have not added test mode, but have
put a CHECK in and the RegisterTranslator tests for bad ranges look for this.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:103: operand_type ==
OperandType::kRegCount16) {
On 2016/01/25 12:12:05, rmcilroy wrote:
> Should this not be checked in OperandIsValid in bytecode array builder instead
> of here? It seems a bit strange to split this up between here and bytecode
array
> builder

Done.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:133: bool
RegisterTranslator::OperandIsMoveable(Bytecode bytecode,
On 2016/01/25 12:12:05, rmcilroy wrote:
> This could be exposed as part of the RegisterMover delegate to keep the logic
of
> which bytecodes are moveable and whether a reg operand followed by a regcount
> operand means a reg range in there.

Done.

https://codereview.chromium.org/1613163002/diff/160001/src/interpreter/regist...
src/interpreter/register-translator.cc:199: // TODO(oth): Move translation
window in this scenario.
On 2016/01/25 12:12:05, rmcilroy wrote:
> Explain why (i.e., the translation window causes a large frame size).

Done.

[rmcilroy, 2016-01-26 11:01:45]

A few more nits, but LGTM. Land it before you change your mind! ;)

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:382: RegisterIsValid(to,
OperandType::kReg8));
Are these DCHECKs needed any longer, since they will be checked in
OperandIsValid?

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:397: // Move bytecodes modify the
stack. Checking validity is essential.
nit- "Move bytecodes modify the stack, so it is essential that we check the
validity of the operation to ensure we don't corrupt the stack." or similar

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:428: //
DCHECK(Bytecodes::IsRegisterOperand(bytecode, operand_index));
Could we do this bytecode right at the top of the function (and avoid the else
branch, just return false at the end of function)?

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1389: return false;
The DCHECK below looks like dead code?

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1390: DCHECK(operand_index != 0);
Dead code?

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1394: if (operand_index > 0) {
Just DCHECK operand_index > 0 here (and below) ?

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1413: previous_operand_type ==
OperandType::kReg16) &&
Should the kReg16 be valid here? Maybe kRegCount8 should only be valid with
kReg8 ?

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:284: void
MoveRegisterUntranslated(Register from, Register to) override;
nit - add a comment that these implement the RegisterMover class

https://codereview.chromium.org/1613163002/diff/220001/test/unittests/interpr...
File test/unittests/interpreter/register-translator-unittest.cc (right):

https://codereview.chromium.org/1613163002/diff/220001/test/unittests/interpr...
test/unittests/interpreter/register-translator-unittest.cc:34:
(moves_.top().first.is_valid() && moves_.top().second.is_valid() &&
nit - pull out moves_.top() as a local

[oth, 2016-01-26 13:39:57]

Comments incorporated.

One seemed to better to leave as-is and one seemed to be a dupe of the comment 2
lines above.

Thanks

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:382: RegisterIsValid(to,
OperandType::kReg8));
On 2016/01/26 11:01:45, rmcilroy wrote:
> Are these DCHECKs needed any longer, since they will be checked in
> OperandIsValid?

Done.

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:397: // Move bytecodes modify the
stack. Checking validity is essential.
On 2016/01/26 11:01:45, rmcilroy wrote:
> nit- "Move bytecodes modify the stack, so it is essential that we check the
> validity of the operation to ensure we don't corrupt the stack." or similar

Done.

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:428: //
DCHECK(Bytecodes::IsRegisterOperand(bytecode, operand_index));
On 2016/01/26 11:01:45, rmcilroy wrote:
> Could we do this bytecode right at the top of the function (and avoid the else
> branch, just return false at the end of function)?

Re-ordered. Test not necessary here, comment removed.

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1389: return false;
On 2016/01/26 11:01:45, rmcilroy wrote:
> The DCHECK below looks like dead code?

Done. Rebase or re-org snafu. Looks like it once belonged in a case below.

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1390: DCHECK(operand_index != 0);
On 2016/01/26 11:01:45, rmcilroy wrote:
> Dead code?

Assuming this is flagging the same dead code as the previous comment.

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1394: if (operand_index > 0) {
On 2016/01/26 11:01:45, rmcilroy wrote:
> Just DCHECK operand_index > 0 here (and below) ? 

Leaving as-is. This method should be consistent and return true or false for all
validity conditions. It's use is wrapped by a DCHECK.

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1413: previous_operand_type ==
OperandType::kReg16) &&
On 2016/01/26 11:01:45, rmcilroy wrote:
> Should the kReg16 be valid here? Maybe kRegCount8 should only be valid with
> kReg8 ?

Removed kReg16.

CallRuntimeWide and CallRuntimeForPairWide have MaybeReg16 and a RegCount8.

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:284: void
MoveRegisterUntranslated(Register from, Register to) override;
On 2016/01/26 11:01:45, rmcilroy wrote:
> nit - add a comment that these implement the RegisterMover class

Done.

https://codereview.chromium.org/1613163002/diff/220001/test/unittests/interpr...
File test/unittests/interpreter/register-translator-unittest.cc (right):

https://codereview.chromium.org/1613163002/diff/220001/test/unittests/interpr...
test/unittests/interpreter/register-translator-unittest.cc:34:
(moves_.top().first.is_valid() && moves_.top().second.is_valid() &&
On 2016/01/26 11:01:45, rmcilroy wrote:
> nit - pull out moves_.top() as a local

Done.

[oth, 2016-01-26 13:50:27]

Description was changed from

==========
[interpreter] Wide register support.

This increases the size of register operands to be 16-bit.

Not all bytecodes have wide register variants, so when they are
needed a register translator will copy them into a small area
reserved at the top of the 8-bit register range and these registers
are supplied as arguments to the bytecode with 8-bit operands.

This is non-intrusive for typical bytecode where the number of
registers is less than 120. For bytecodes with wide register
operands (above the window) their index needs to be translated
to avoid the reserved translation window.

Enables splay.js to run in Octane and a handful of mjsunit tests.

BUG=v8:4675
LOG=NO
==========

to

==========
[interpreter] Wide register support.

This increases the size of register operands to be 16-bit.

Not all bytecodes have wide register variants, so when they are
needed a register translator will copy them into a small area
reserved at the top of the 8-bit register range and these registers
are supplied as arguments to the bytecode with 8-bit operands.

This is non-intrusive for typical bytecode where the number of
registers is less than 120. For bytecodes with wide register
operands (above the window) their index needs to be translated
to avoid the reserved translation window.

Enables splay.js to run in Octane and a handful of mjsunit tests.

BUG=v8:4280,v8:4675
LOG=NO
==========

[oth, 2016-01-26 13:53:51]

The CQ bit was checked by oth@chromium.org

[oth, 2016-01-26 13:53:51]

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/1613163002/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-26 13:54:00]

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

[commit-bot: I haz the power, 2016-01-26 13:55:34]

Description was changed from

==========
[interpreter] Wide register support.

This increases the size of register operands to be 16-bit.

Not all bytecodes have wide register variants, so when they are
needed a register translator will copy them into a small area
reserved at the top of the 8-bit register range and these registers
are supplied as arguments to the bytecode with 8-bit operands.

This is non-intrusive for typical bytecode where the number of
registers is less than 120. For bytecodes with wide register
operands (above the window) their index needs to be translated
to avoid the reserved translation window.

Enables splay.js to run in Octane and a handful of mjsunit tests.

BUG=v8:4280,v8:4675
LOG=NO
==========

to

==========
[interpreter] Wide register support.

This increases the size of register operands to be 16-bit.

Not all bytecodes have wide register variants, so when they are
needed a register translator will copy them into a small area
reserved at the top of the 8-bit register range and these registers
are supplied as arguments to the bytecode with 8-bit operands.

This is non-intrusive for typical bytecode where the number of
registers is less than 120. For bytecodes with wide register
operands (above the window) their index needs to be translated
to avoid the reserved translation window.

Enables splay.js to run in Octane and a handful of mjsunit tests.

BUG=v8:4280,v8:4675
LOG=NO
==========

[commit-bot: I haz the power, 2016-01-26 13:55:34]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-01-26 13:56:21]

Description was changed from

==========
[interpreter] Wide register support.

This increases the size of register operands to be 16-bit.

Not all bytecodes have wide register variants, so when they are
needed a register translator will copy them into a small area
reserved at the top of the 8-bit register range and these registers
are supplied as arguments to the bytecode with 8-bit operands.

This is non-intrusive for typical bytecode where the number of
registers is less than 120. For bytecodes with wide register
operands (above the window) their index needs to be translated
to avoid the reserved translation window.

Enables splay.js to run in Octane and a handful of mjsunit tests.

BUG=v8:4280,v8:4675
LOG=NO
==========

to

==========
[interpreter] Wide register support.

This increases the size of register operands to be 16-bit.

Not all bytecodes have wide register variants, so when they are
needed a register translator will copy them into a small area
reserved at the top of the 8-bit register range and these registers
are supplied as arguments to the bytecode with 8-bit operands.

This is non-intrusive for typical bytecode where the number of
registers is less than 120. For bytecodes with wide register
operands (above the window) their index needs to be translated
to avoid the reserved translation window.

Enables splay.js to run in Octane and a handful of mjsunit tests.

BUG=v8:4280,v8:4675
LOG=NO

Committed: https://crrev.com/19df7a20f0d50e3b6a8db39c39ee1cd157c56d25
Cr-Commit-Position: refs/heads/master@{#33516}
==========

[commit-bot: I haz the power, 2016-01-26 13:56:22]

Patchset 15 (id:??) landed as
https://crrev.com/19df7a20f0d50e3b6a8db39c39ee1cd157c56d25
Cr-Commit-Position: refs/heads/master@{#33516}

[rmcilroy, 2016-01-26 14:08:07]

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1390: DCHECK(operand_index != 0);
On 2016/01/26 13:39:56, oth wrote:
> On 2016/01/26 11:01:45, rmcilroy wrote:
> > Dead code?
> 
> Assuming this is flagging the same dead code as the previous comment.

Yes it was. Weirdly the tool told me it failed to create this comment multiple
times, so I created the one above :).

https://codereview.chromium.org/1613163002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1394: if (operand_index > 0) {
On 2016/01/26 13:39:57, oth wrote:
> On 2016/01/26 11:01:45, rmcilroy wrote:
> > Just DCHECK operand_index > 0 here (and below) ? 
> 
> Leaving as-is. This method should be consistent and return true or false for
all
> validity conditions. It's use is wrapped by a DCHECK.

Acknowledged.