[interpreter] Add support for scalable operands.

[interpreter] Add support for scalable operands.

This change introduces wide prefix bytecodes to support wide (16-bit)
and extra-wide (32-bit) operands. It retires the previous
wide-bytecodes and reduces the number of operand types.

Operands are now either scalable or fixed size. Scalable operands
increase in width when a bytecode is prefixed with wide or extra-wide.

The bytecode handler table is extended to 256*3 entries. The
first 256 entries are used for bytecodes with 8-bit operands,
the second 256 entries are used for bytecodes with operands that
scale to 16-bits, and the third group of 256 entries are used for
bytecodes with operands that scale to 32-bits.

LOG=N
BUG=v8:4747, v8:4280
Committed: https://crrev.com/48d082af38f692a27b4fb8caf28dbb426d69424a
Cr-Commit-Position: refs/heads/master@{#34955}

[oth, 2016-03-09 16:52:37]

oth@chromium.org changed reviewers:
+ danno@chromium.org, mstarzinger@chromium.org, mythria@chromium.org,
rmcilroy@chromium.org, ssanfilippo@chromium.org, yangguo@chromium.org

[oth, 2016-03-09 16:52:38]

Hi all

Sharing this CL for high-level comments on the general approach of changing to
prefix bytecodes for wider operands. There's enough here to run all of Octane 2
and it passes existing tests on x64. The *.golden files give a view of what's
changing in the emitted bytecode.

Yang, this change adapts Bytecodes::GetDebugBreak and the debug.cc to work with
prefix bytecodes.

Stefano, there are changes here to the bytecode expectations tools for prefix
bytecodes.

Michi, bytecode-graph-generator loses the visit methods for the old wide
bytecodes.

Ross, Mythri. Lots of changes in the interpreter directory including losing the
register translation (yay!).

TODO list:
- Fix up interpreter-assembler tests.
- Some clean-up in bytecodes.cc
- Fix GCC/MSVC compilation nits that are bound to exist.
- Restore direct lookup of bytecode sizes in bytecodes.cc (propose doing this in
a separate CL).

Comments appreciated.

Thanks
Orion

[oth, 2016-03-10 12:31:34]

Description was changed from

==========
[interpreter] Add support for scalable operands.

This change introduces wide prefix bytecodes to support wide (16-bit)
and extra-wide (32-bit) operands. It retires the previous
wide-bytecodes and reduces the number of operand types.

Operands are now either scalable or fixed size. Scalable operands
increase in width when a bytecode is prefixed with wide or extra-wide.

LOG=N
BUG=v8:4747,v8:4280
==========

to

==========
[interpreter] Add support for scalable operands.

This change introduces wide prefix bytecodes to support wide (16-bit)
and extra-wide (32-bit) operands. It retires the previous
wide-bytecodes and reduces the number of operand types.

Operands are now either scalable or fixed size. Scalable operands
increase in width when a bytecode is prefixed with wide or extra-wide.

The bytecode handler table is extended to 256*3 entries. The
first 256 entries are used for bytecodes with 8-bit operands,
the second 256 entries are used for bytecodes with operands that
scale to 16-bits, and the third group of 256 entries are used for
bytecodes with operands that scale to 32-bits. 

LOG=N
BUG=v8:4747,v8:4280
==========

[rmcilroy, 2016-03-10 16:45:38]

Overall looks pretty great. I've got a couple of high level comments and a few
minor suggestions, will look in more detail once it's more finished, but I'm
happy with the overall approach.

https://codereview.chromium.org/1783483002/diff/100001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:225: if
(interpreter::Bytecodes::GetPrefixBytecodeScale(bytecode) > 1) {
Could you pull out the logic for getting the current bytecode and operand scale
to a helper function and use that throughout.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:133: switch (bytecode) {
Could we make this DCHECK if it is passed a non-prefix bytecode.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:346: bool
Bytecodes::IsRuntimeIdOperandType(OperandType operand_type) {
Wondering whether these IsXXXOperandType are necessary now given they all only
check one operand type now. I'm fine with keeping them though.

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

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:24: V(RegPair, OperandSize::kByte,
OperandTypeTrait::kScalable)
Can we get rid of the OperandSize attribute in these lists now (i.e., have
OperandTypeTrait::kNone, OperandTypeTrait::kFixedByte
OperandTypeTrait::kFixedShort and OperandTypeTrait::kScaled)?

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:364: static int GetPrefixBytecodeScale(Bytecode
bytecode);
Could we have this return operand sizes instead (i.e.,
GetScaledOperandSizeForPrefix(Bytecode bytecode). I think this would be doable
for all your current uses of this, and then you could adapt the functions below
to take OperandSize scaled_operand_size) as parameters. Not sure if this is
cleaner, just a though, happy to discuss further.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:455: // Returns true if |operand_type| is a
runtime-id id operand (kRuntimeId).
remove second "id"

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:24: // Constructor for illegal bytecode
codepoints.
Could we just add a kIllegal bytecode to avoid having to special case the
constructor? That would mean we could return it on unreachable codepaths instead
of static_cast<Bytecode>(-1).

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:146: void RedispatchWide();
Could we do the logic for this in interpreter.cc and just reuse
DispatchToBytecodeHandler instead?

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:41: int row = i * (kMaxUInt8 + 1) +
Bytecodes::ToByte(Bytecode::k##Name); \
Could you pull this out into a private helper function and share the logic with
GetBytecodeHandler

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:44: Bytecodes::IsDebugBreak(Bytecode::k##Name)) {
                    \
Is the IsDebugBreak necessary? Isn't it only the DebugBreak's with scalable
operands that we want to generate wide variants for anyway?

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:61: dispatch_table_[p] =
*invalid_bytecode_handler;
How about just setting the whole dispatch table with invalid_bytecode_handler
initially, then you don't need to fill in the remainder at the end or set it to
invalid in the loop above if it isn't scalable?

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:69: switch (operand_scale) {
I don't think you need the switch for this, you should be able to just use the
same logic as above

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:85: return bytecode_handler !=
invalid_bytecode_handler ? bytecode_handler
Why is this necessary? We shouldn't ever be asking for an entry which has an
invalid bytecode handler here, can't we just DCHECK_NE(bytecode_handler,
invalid_bytecode_handler) ?

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.h:137: int operand_scale() const { return
operand_scale_; }
I'd rather not have these as object fields since they are only valid for certain
points during building the bytecode handlers. Could you just set operand_scale
(or even better a scaled_operand_size) on the interpreter-assembler instead - I
don't think it's needed anywhere else in interpreter.cc

https://codereview.chromium.org/1783483002/diff/100001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/log.cc#newcode1617
src/log.cc:1617: std::string bytecode_name =
interpreter::Bytecodes::ToString(bytecode);
How about making Bytecode::ToString take a width too and also do the prepending
of .Wide / .ExtraWide itself?

https://codereview.chromium.org/1783483002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/objects.cc#newcode...
src/objects.cc:15066: int prefix_offset = 0;
It might be time to rework this so that it uses the BytecodeIterator somehow - I
think it could be much simpler just using that, WDYT? This could maybe be a
seperate (preceeding) CL to do this and hopefully after that's happened nothing
much would need to change in this CL.

[danno, 2016-03-10 17:23:40]

Generally, I really, really like the direction this is going. 

At this point, only +1 to "pulling out the logic" that is duplicated in several
places into separate functions.

[rmcilroy, 2016-03-11 11:35:20]

https://codereview.chromium.org/1783483002/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.h (right):

https://codereview.chromium.org/1783483002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.h:24: void set_current_offset(int
offset) { bytecode_offset_ = offset; }
Just though about this - we need to do something for the set_current_offset
call, currently it doesn't update the scale appropriately. Currently it is only
used by Runtime_TraceBytecode*, so maybe we just get rid of it and have
Runtime_TraceBytecode advance until it reaches the correct location. WDYT?

[oth, 2016-03-11 16:26:13]

Thanks, still working through these, but have checked off a few today. Running
tests on the bots.

https://codereview.chromium.org/1783483002/diff/100001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/debug/debug.cc#new...
src/debug/debug.cc:225: if
(interpreter::Bytecodes::GetPrefixBytecodeScale(bytecode) > 1) {
On 2016/03/10 16:45:37, rmcilroy wrote:
> Could you pull out the logic for getting the current bytecode and operand
scale
> to a helper function and use that throughout.

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:133: switch (bytecode) {
On 2016/03/10 16:45:37, rmcilroy wrote:
> Could we make this DCHECK if it is passed a non-prefix bytecode.

Not really, this is one of the main ways of determining whether there is a
prefix rather than checking if it's a *Wide and then querying again for the
scaling factor.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:346: bool
Bytecodes::IsRuntimeIdOperandType(OperandType operand_type) {
On 2016/03/10 16:45:37, rmcilroy wrote:
> Wondering whether these IsXXXOperandType are necessary now given they all only
> check one operand type now. I'm fine with keeping them though.

Done. Removed all except the still useful IsRegister... set.

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

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:24: V(RegPair, OperandSize::kByte,
OperandTypeTrait::kScalable)
On 2016/03/10 16:45:37, rmcilroy wrote:
> Can we get rid of the OperandSize attribute in these lists now (i.e., have
> OperandTypeTrait::kNone, OperandTypeTrait::kFixedByte
> OperandTypeTrait::kFixedShort and OperandTypeTrait::kScaled)?

Acknowledged. Working on it with the signed variations.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:364: static int GetPrefixBytecodeScale(Bytecode
bytecode);
On 2016/03/10 16:45:37, rmcilroy wrote:
> Could we have this return operand sizes instead (i.e.,
> GetScaledOperandSizeForPrefix(Bytecode bytecode). I think this would be doable
> for all your current uses of this, and then you could adapt the functions
below
> to take OperandSize scaled_operand_size) as parameters. Not sure if this is
> cleaner, just a though, happy to discuss further.

Ack. Leaving this to last piece to try in the CL. Scaling is inherently
unitless, operand size has units.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:455: // Returns true if |operand_type| is a
runtime-id id operand (kRuntimeId).
On 2016/03/10 16:45:37, rmcilroy wrote:
> remove second "id"

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:24: // Constructor for illegal bytecode
codepoints.
On 2016/03/10 16:45:37, rmcilroy wrote:
> Could we just add a kIllegal bytecode to avoid having to special case the
> constructor? That would mean we could return it on unreachable codepaths
instead
> of static_cast<Bytecode>(-1).

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:146: void RedispatchWide();
On 2016/03/10 16:45:37, rmcilroy wrote:
> Could we do the logic for this in interpreter.cc and just reuse
> DispatchToBytecodeHandler instead?

No problem moving this, but it seems very similar to plain old Dispatch(). The
implementation uses DispatchToBytecodeHandler and is common to both Wide and
ExtraWide bytecodes.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:41: int row = i * (kMaxUInt8 + 1) +
Bytecodes::ToByte(Bytecode::k##Name); \
On 2016/03/10 16:45:38, rmcilroy wrote:
> Could you pull this out into a private helper function and share the logic
with
> GetBytecodeHandler

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:44: Bytecodes::IsDebugBreak(Bytecode::k##Name)) {
                    \
On 2016/03/10 16:45:38, rmcilroy wrote:
> Is the IsDebugBreak necessary? Isn't it only the DebugBreak's with scalable
> operands that we want to generate wide variants for anyway?

Good spot, chaff from an earlier variation on handling debug break.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:61: dispatch_table_[p] =
*invalid_bytecode_handler;
On 2016/03/10 16:45:38, rmcilroy wrote:
> How about just setting the whole dispatch table with invalid_bytecode_handler
> initially, then you don't need to fill in the remainder at the end or set it
to
> invalid in the loop above if it isn't scalable?

Done in spirit - with the Illegal bytecode there's an ordering dependency.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:69: switch (operand_scale) {
On 2016/03/10 16:45:38, rmcilroy wrote:
> I don't think you need the switch for this, you should be able to just use the
> same logic as above

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:85: return bytecode_handler !=
invalid_bytecode_handler ? bytecode_handler
On 2016/03/10 16:45:38, rmcilroy wrote:
> Why is this necessary? We shouldn't ever be asking for an entry which has an
> invalid bytecode handler here, can't we just DCHECK_NE(bytecode_handler,
> invalid_bytecode_handler) ?

It's not clear the caller should know which entries are valid. ie
Logger::LogBytecodeHandlers.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.h:137: int operand_scale() const { return
operand_scale_; }
On 2016/03/10 16:45:38, rmcilroy wrote:
> I'd rather not have these as object fields since they are only valid for
certain
> points during building the bytecode handlers. Could you just set operand_scale
> (or even better a scaled_operand_size) on the interpreter-assembler instead -
I
> don't think it's needed anywhere else in interpreter.cc

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/log.cc#newcode1617
src/log.cc:1617: std::string bytecode_name =
interpreter::Bytecodes::ToString(bytecode);
On 2016/03/10 16:45:38, rmcilroy wrote:
> How about making Bytecode::ToString take a width too and also do the
prepending
> of .Wide / .ExtraWide itself?

Added an overload to do this.

https://codereview.chromium.org/1783483002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/objects.cc#newcode...
src/objects.cc:15066: int prefix_offset = 0;
On 2016/03/10 16:45:38, rmcilroy wrote:
> It might be time to rework this so that it uses the BytecodeIterator somehow -
I
> think it could be much simpler just using that, WDYT? This could maybe be a
> seperate (preceeding) CL to do this and hopefully after that's happened
nothing
> much would need to change in this CL.

Ack.

https://codereview.chromium.org/1783483002/diff/180001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.h (right):

https://codereview.chromium.org/1783483002/diff/180001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.h:24: void set_current_offset(int
offset) { bytecode_offset_ = offset; }
On 2016/03/11 11:35:20, rmcilroy wrote:
> Just though about this - we need to do something for the set_current_offset
> call, currently it doesn't update the scale appropriately. Currently it is
only
> used by Runtime_TraceBytecode*, so maybe we just get rid of it and have
> Runtime_TraceBytecode advance until it reaches the correct location. WDYT?

Good catch.

The latest rev does multiple Advance()'s if the previous bytecode could be
interpreted as a widening prefix, otherwise just sets the iterator position
accordingly.

It's a bit ugly at the moment as we filter out the tracing for just the widening
bytecode. We might do this earlier in the call stack as the changes progress.

[oth, 2016-03-15 09:58:26]

oth@chromium.org changed reviewers:
- danno@chromium.org

[oth, 2016-03-15 09:58:28]

Folks, when you're ready, it'd be good to get review comments going for this CL.
It's a big CL, but mostly under src/interpreter.

Michi, please take a look at code removed from compiler.

Mythri, comments on any and all of this are appreciated.

Ross, thanks for the earlier comments. I don't feel two of the suggestions work
out very well (see below), push if you feel strongly about them otherwise not
keen to action them.

Stefano, bytecode expections generator has been updated for wide operands. PTAL.

Yang, please take a look at the debug changes.

Thanks

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

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:24: V(RegPair, OperandSize::kByte,
OperandTypeTrait::kScalable)
On 2016/03/11 16:26:12, oth wrote:
> On 2016/03/10 16:45:37, rmcilroy wrote:
> > Can we get rid of the OperandSize attribute in these lists now (i.e., have
> > OperandTypeTrait::kNone, OperandTypeTrait::kFixedByte
> > OperandTypeTrait::kFixedShort and OperandTypeTrait::kScaled)?
> 
> Acknowledged. Working on it with the signed variations.

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:364: static int GetPrefixBytecodeScale(Bytecode
bytecode);
On 2016/03/11 16:26:12, oth wrote:
> On 2016/03/10 16:45:37, rmcilroy wrote:
> > Could we have this return operand sizes instead (i.e.,
> > GetScaledOperandSizeForPrefix(Bytecode bytecode). I think this would be
doable
> > for all your current uses of this, and then you could adapt the functions
> below
> > to take OperandSize scaled_operand_size) as parameters. Not sure if this is
> > cleaner, just a though, happy to discuss further.
> 
> Ack. Leaving this to last piece to try in the CL. Scaling is inherently
> unitless, operand size has units.

We have a difference of opinion here. I pushed this through and it ripples all
over the CL. It seems more confusing as there are two interpretations of
OperandSize and in some places the code has two OperandSizes - the non-scaled
size and the scaled size. And it doesn't work if we ever end up scaling operands
any size other than byte, but maybe that won't happen.

Prefer it as something after this CL that someone else can pursue - it's not a
hard change.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:146: void RedispatchWide();
On 2016/03/11 16:26:12, oth wrote:
> On 2016/03/10 16:45:37, rmcilroy wrote:
> > Could we do the logic for this in interpreter.cc and just reuse
> > DispatchToBytecodeHandler instead?
> 
> No problem moving this, but it seems very similar to plain old Dispatch(). The
> implementation uses DispatchToBytecodeHandler and is common to both Wide and
> ExtraWide bytecodes.
> 

Looking into this, more of the interpreter assemblers internals (bytecode
offset, bytecode array, dispatch table) need to be exposed to support this. This
information seems to have the right accessibility as-is.

[oth, 2016-03-16 11:20:56]

On 2016/03/15 09:58:28, oth wrote:
> Folks, when you're ready, it'd be good to get review comments going for this
CL.
> It's a big CL, but mostly under src/interpreter.
> 
> Michi, please take a look at code removed from compiler.
> 
> Mythri, comments on any and all of this are appreciated.
> 
> Ross, thanks for the earlier comments. I don't feel two of the suggestions
work
> out very well (see below), push if you feel strongly about them otherwise not
> keen to action them.
> 
> Stefano, bytecode expections generator has been updated for wide operands.
PTAL.
> 
> Yang, please take a look at the debug changes.
> 
> Thanks
> 
>
https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecodes.h:24: V(RegPair, OperandSize::kByte,
> OperandTypeTrait::kScalable)
> On 2016/03/11 16:26:12, oth wrote:
> > On 2016/03/10 16:45:37, rmcilroy wrote:
> > > Can we get rid of the OperandSize attribute in these lists now (i.e., have
> > > OperandTypeTrait::kNone, OperandTypeTrait::kFixedByte
> > > OperandTypeTrait::kFixedShort and OperandTypeTrait::kScaled)?
> > 
> > Acknowledged. Working on it with the signed variations.
> 
> Done.
> 
>
https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
> src/interpreter/bytecodes.h:364: static int GetPrefixBytecodeScale(Bytecode
> bytecode);
> On 2016/03/11 16:26:12, oth wrote:
> > On 2016/03/10 16:45:37, rmcilroy wrote:
> > > Could we have this return operand sizes instead (i.e.,
> > > GetScaledOperandSizeForPrefix(Bytecode bytecode). I think this would be
> doable
> > > for all your current uses of this, and then you could adapt the functions
> > below
> > > to take OperandSize scaled_operand_size) as parameters. Not sure if this
is
> > > cleaner, just a though, happy to discuss further.
> > 
> > Ack. Leaving this to last piece to try in the CL. Scaling is inherently
> > unitless, operand size has units.
> 
> We have a difference of opinion here. I pushed this through and it ripples all
> over the CL. It seems more confusing as there are two interpretations of
> OperandSize and in some places the code has two OperandSizes - the non-scaled
> size and the scaled size. And it doesn't work if we ever end up scaling
operands
> any size other than byte, but maybe that won't happen.
> 
> Prefer it as something after this CL that someone else can pursue - it's not a
> hard change.
> 
>
https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
> File src/interpreter/interpreter-assembler.h (right):
> 
>
https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
> src/interpreter/interpreter-assembler.h:146: void RedispatchWide();
> On 2016/03/11 16:26:12, oth wrote:
> > On 2016/03/10 16:45:37, rmcilroy wrote:
> > > Could we do the logic for this in interpreter.cc and just reuse
> > > DispatchToBytecodeHandler instead?
> > 
> > No problem moving this, but it seems very similar to plain old Dispatch().
The
> > implementation uses DispatchToBytecodeHandler and is common to both Wide and
> > ExtraWide bytecodes.
> > 
> 
> Looking into this, more of the interpreter assemblers internals (bytecode
> offset, bytecode array, dispatch table) need to be exposed to support this.
This
> information seems to have the right accessibility as-is.

There's an issue with the debugger support. Probably not worth looking at this
just now.

[rmcilroy, 2016-03-16 11:55:54]

A couple more high level comments.

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

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:364: static int GetPrefixBytecodeScale(Bytecode
bytecode);
On 2016/03/15 09:58:28, oth wrote:
> On 2016/03/11 16:26:12, oth wrote:
> > On 2016/03/10 16:45:37, rmcilroy wrote:
> > > Could we have this return operand sizes instead (i.e.,
> > > GetScaledOperandSizeForPrefix(Bytecode bytecode). I think this would be
> doable
> > > for all your current uses of this, and then you could adapt the functions
> > below
> > > to take OperandSize scaled_operand_size) as parameters. Not sure if this
is
> > > cleaner, just a though, happy to discuss further.
> > 
> > Ack. Leaving this to last piece to try in the CL. Scaling is inherently
> > unitless, operand size has units.
> 
> We have a difference of opinion here. I pushed this through and it ripples all
> over the CL. It seems more confusing as there are two interpretations of
> OperandSize and in some places the code has two OperandSizes - the non-scaled
> size and the scaled size. And it doesn't work if we ever end up scaling
operands
> any size other than byte, but maybe that won't happen.
> 
> Prefer it as something after this CL that someone else can pursue - it's not a
> hard change.

Ok, can we make this an enum then? It doesn't seem to be used anywhere to
directly scale anything, and is mostly used in switch statements or comparison
with "operand_scale > 1" which I think would be more appropriate as an enum and
a "RequiresBytecodePrefix(OperandScale ...)" helper function.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:146: void RedispatchWide();
On 2016/03/15 09:58:28, oth wrote:
> On 2016/03/11 16:26:12, oth wrote:
> > On 2016/03/10 16:45:37, rmcilroy wrote:
> > > Could we do the logic for this in interpreter.cc and just reuse
> > > DispatchToBytecodeHandler instead?
> > 
> > No problem moving this, but it seems very similar to plain old Dispatch().
The
> > implementation uses DispatchToBytecodeHandler and is common to both Wide and
> > ExtraWide bytecodes.
> > 
> 
> Looking into this, more of the interpreter assemblers internals (bytecode
> offset, bytecode array, dispatch table) need to be exposed to support this.
This
> information seems to have the right accessibility as-is.

Ok that's fine. Please rename as DispatchWide though since you aren't
redispatching the current bytecode, just ensuring that the next bytecode is
treated as wide.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:85: return bytecode_handler !=
invalid_bytecode_handler ? bytecode_handler
On 2016/03/11 16:26:12, oth wrote:
> On 2016/03/10 16:45:38, rmcilroy wrote:
> > Why is this necessary? We shouldn't ever be asking for an entry which has an
> > invalid bytecode handler here, can't we just DCHECK_NE(bytecode_handler,
> > invalid_bytecode_handler) ?
> 
> It's not clear the caller should know which entries are valid. ie
> Logger::LogBytecodeHandlers.

Ok, I still think we should be returning the actual invalid bytecode handler
here rather than nullptr though - this shouldn't be an issue for the logger I
don't think.

https://codereview.chromium.org/1783483002/diff/330001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/debug/debug.cc#new...
src/debug/debug.cc:221: class BytecodePrefixHelper final {
As discussed offline, I think we could get rid of the need for the debugger to
need to know about how to read prefix bytecodes if we instead set the breakpoint
on the actual prefix bytecode instead of the subsiquent bytecode. This would
mean we would need to introduce two additional debug break bytecodes which
represent wide and extra wide prefixes, but I think we can git rid of some other
scaled debug break bytecodes anyway.

https://codereview.chromium.org/1783483002/diff/330001/src/debug/debug.cc#new...
src/debug/debug.cc:1612: prefix_helper.bytecode(),
prefix_helper.operand_scale());
As discussed offline, I don't think this would work since the debug break is
after the prefix bytecode so we are returning a non-scaled bytecode handler
here, even when the operation should be scaled. The approach of setting the
breakpoint on the prefix bytecode mentioned above would solve this I think.

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

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:26: }
I would prefer this as: 

operand_scale_ = 1;
if (IsPrefixBytecode(bytecode_) {
  operand_scale = GetOperandScaleForPrefixBytecode(bytecode_)
  bytecode = ....
}

rather than relying on the fact that a non-prefix bytecode returns 1 for
GetPrefixBytecodeScale (I would rather GetOperandScaleForPrefixBytecode DCHECKs
on non prefix bytecodes).

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1161: // The size of reg1 is
immaterial.
Good catch! :)

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:27: void
BytecodeArrayIterator::update_operand_scale() {
inline into Advance if we removing set_current_offset (see below).

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:54: void
BytecodeArrayIterator::set_current_offset(int offset) {
I think this set_current_offset is dangerous. Could we just get rid of it for
now and always do the slow scan for now, and then we can add something back if
we decide it's causing bytecode tracing to be too slow.

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:547:
DCHECK(base::bits::IsPowerOfTwo32(operand_scale_));
Not sure why this DCHECK is needed here, the code here doesn't rely on it.

[mythria, 2016-03-16 13:32:11]

Looks great. I was more trying to understand the code, than review it :). I have
just one comment. I haven't looked at tests yet.

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/consta...
File src/interpreter/constant-array-builder.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:62: zone, k8BitCapacity,
k16BitCapacity, OperandSize::kQuad);
I think this should go from k16BitCapacity to k32BitCapacity.

[oth, 2016-03-17 13:48:39]

Thanks for the comments. The code tries to address them all or should have a
comment below. PTAL. Phew!

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:133: switch (bytecode) {
On 2016/03/11 16:26:12, oth wrote:
> On 2016/03/10 16:45:37, rmcilroy wrote:
> > Could we make this DCHECK if it is passed a non-prefix bytecode.
> 
> Not really, this is one of the main ways of determining whether there is a
> prefix rather than checking if it's a *Wide and then querying again for the
> scaling factor.

Done.

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

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.h:364: static int GetPrefixBytecodeScale(Bytecode
bytecode);
On 2016/03/16 11:55:53, rmcilroy wrote:
> On 2016/03/15 09:58:28, oth wrote:
> > On 2016/03/11 16:26:12, oth wrote:
> > > On 2016/03/10 16:45:37, rmcilroy wrote:
> > > > Could we have this return operand sizes instead (i.e.,
> > > > GetScaledOperandSizeForPrefix(Bytecode bytecode). I think this would be
> > doable
> > > > for all your current uses of this, and then you could adapt the
functions
> > > below
> > > > to take OperandSize scaled_operand_size) as parameters. Not sure if this
> is
> > > > cleaner, just a though, happy to discuss further.
> > > 
> > > Ack. Leaving this to last piece to try in the CL. Scaling is inherently
> > > unitless, operand size has units.
> > 
> > We have a difference of opinion here. I pushed this through and it ripples
all
> > over the CL. It seems more confusing as there are two interpretations of
> > OperandSize and in some places the code has two OperandSizes - the
non-scaled
> > size and the scaled size. And it doesn't work if we ever end up scaling
> operands
> > any size other than byte, but maybe that won't happen.
> > 
> > Prefer it as something after this CL that someone else can pursue - it's not
a
> > hard change.
> 
> Ok, can we make this an enum then? It doesn't seem to be used anywhere to
> directly scale anything, and is mostly used in switch statements or comparison
> with "operand_scale > 1" which I think would be more appropriate as an enum
and
> a "RequiresBytecodePrefix(OperandScale ...)" helper function.

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:146: void RedispatchWide();
On 2016/03/16 11:55:54, rmcilroy wrote:
> On 2016/03/15 09:58:28, oth wrote:
> > On 2016/03/11 16:26:12, oth wrote:
> > > On 2016/03/10 16:45:37, rmcilroy wrote:
> > > > Could we do the logic for this in interpreter.cc and just reuse
> > > > DispatchToBytecodeHandler instead?
> > > 
> > > No problem moving this, but it seems very similar to plain old Dispatch().
> The
> > > implementation uses DispatchToBytecodeHandler and is common to both Wide
and
> > > ExtraWide bytecodes.
> > > 
> > 
> > Looking into this, more of the interpreter assemblers internals (bytecode
> > offset, bytecode array, dispatch table) need to be exposed to support this.
> This
> > information seems to have the right accessibility as-is.
> 
> Ok that's fine. Please rename as DispatchWide though since you aren't
> redispatching the current bytecode, just ensuring that the next bytecode is
> treated as wide.

Done.

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/100001/src/interpreter/interp...
src/interpreter/interpreter.cc:85: return bytecode_handler !=
invalid_bytecode_handler ? bytecode_handler
On 2016/03/16 11:55:54, rmcilroy wrote:
> On 2016/03/11 16:26:12, oth wrote:
> > On 2016/03/10 16:45:38, rmcilroy wrote:
> > > Why is this necessary? We shouldn't ever be asking for an entry which has
an
> > > invalid bytecode handler here, can't we just DCHECK_NE(bytecode_handler,
> > > invalid_bytecode_handler) ?
> > 
> > It's not clear the caller should know which entries are valid. ie
> > Logger::LogBytecodeHandlers.
> 
> Ok, I still think we should be returning the actual invalid bytecode handler
> here rather than nullptr though - this shouldn't be an issue for the logger I
> don't think.

The underlying issue is just that the logger wants to write the name of the
bytecode. There is a sanity check that the bytecode is valid when checking the
name, that breaks and it's probably a useful check.

The interpreter now exposes a BytecodeHasHandler method to decide which handlers
to generate and for the logger to query.

https://codereview.chromium.org/1783483002/diff/330001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/debug/debug.cc#new...
src/debug/debug.cc:221: class BytecodePrefixHelper final {
On 2016/03/16 11:55:54, rmcilroy wrote:
> As discussed offline, I think we could get rid of the need for the debugger to
> need to know about how to read prefix bytecodes if we instead set the
breakpoint
> on the actual prefix bytecode instead of the subsiquent bytecode. This would
> mean we would need to introduce two additional debug break bytecodes which
> represent wide and extra wide prefixes, but I think we can git rid of some
other
> scaled debug break bytecodes anyway.

Done.

https://codereview.chromium.org/1783483002/diff/330001/src/debug/debug.cc#new...
src/debug/debug.cc:1612: prefix_helper.bytecode(),
prefix_helper.operand_scale());
On 2016/03/16 11:55:54, rmcilroy wrote:
> As discussed offline, I don't think this would work since the debug break is
> after the prefix bytecode so we are returning a non-scaled bytecode handler
> here, even when the operation should be scaled. The approach of setting the
> breakpoint on the prefix bytecode mentioned above would solve this I think.

Done.

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

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:26: }
On 2016/03/16 11:55:54, rmcilroy wrote:
> I would prefer this as: 
> 
> operand_scale_ = 1;
> if (IsPrefixBytecode(bytecode_) {
>   operand_scale = GetOperandScaleForPrefixBytecode(bytecode_)
>   bytecode = ....
> }
> 
> rather than relying on the fact that a non-prefix bytecode returns 1 for
> GetPrefixBytecodeScale (I would rather GetOperandScaleForPrefixBytecode
DCHECKs
> on non prefix bytecodes). 

Done.

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:27: void
BytecodeArrayIterator::update_operand_scale() {
On 2016/03/16 11:55:54, rmcilroy wrote:
> inline into Advance if we removing set_current_offset (see below).

The constructor uses it too and will break without duplicating the same code.

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:54: void
BytecodeArrayIterator::set_current_offset(int offset) {
On 2016/03/16 11:55:54, rmcilroy wrote:
> I think this set_current_offset is dangerous. Could we just get rid of it for
> now and always do the slow scan for now, and then we can add something back if
> we decide it's causing bytecode tracing to be too slow.

Done. Possibly better to put valid positions into a bitmap if a non-sequential
scan is requested. This only occurs in tracing. It's not a much work, but not
for this CL.

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/consta...
File src/interpreter/constant-array-builder.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:62: zone, k8BitCapacity,
k16BitCapacity, OperandSize::kQuad);
On 2016/03/16 13:32:11, mythria wrote:
> I think this should go from k16BitCapacity to k32BitCapacity.

Done.

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/330001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:547:
DCHECK(base::bits::IsPowerOfTwo32(operand_scale_));
On 2016/03/16 11:55:54, rmcilroy wrote:
> Not sure why this DCHECK is needed here, the code here doesn't rely on it.

Done.

[rmcilroy, 2016-03-17 17:30:51]

Looks great, just minor comments.

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

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:62: switch (operand_size) {
I think you could use DecodeSignedOperand here to avoid duplicating code.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:78: MUST_USE_RESULT int32_t
GetUnsignedOperand(int operand_index) const {
should this return uint32_t instead?

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:92: DCHECK(operand_size >=
OperandSize::kByte &&
Just DCHECK_NE(operand_size, OperandSize::kNone) should work

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:804: delta -= 1;
nit - add a comment that this is compensating for the prefix bytecode which will
be emitted before the jump bytecode.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1470: OperandScale operand_scale =
static_cast<OperandScale>(result);
Could you add a STATIC_ASSERT that casting OperandSize to OperandScale gives the
expected results.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1478: return
static_cast<uint32_t>(reg.ToOperand());
Could we just make reg.ToOperand return a uint32_t and remove this method (same
for FromOperand below)?

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

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:275: static OperandSize
SizeForRegisterOperand(Register reg);
nit - newline above

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:279: static uint32_t
RegisterOperand(Register reg);
nit - newline above. Also, can these all be private (search in this CL doesn't
work too well, but I can't see uses outside BytecodeArrayBuilder)?

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:72: switch
(Bytecodes::SizeOfOperand(operand_type, current_operand_scale())) {
I think you could use DecodeUnsignedOperand here to avoid duplicating code.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:97: switch
(Bytecodes::SizeOfOperand(operand_type, current_operand_scale())) {
I think you could use DecodeSignedOperand here to avoid duplicating code.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:124:
DCHECK(Bytecodes::GetOperandType(current_bytecode(), operand_index) ==
DCHECK_EQ

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:132: DCHECK(operand_type ==
OperandType::kIdx);
DCHECK_EQ

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.h (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.h:50: void update_operand_scale();
nit - UpdateOperandScale  (not a getter or setter so should be CapsCase)

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:127: int delta = static_cast<int>(operand_size);
Could you add a comment to OperandSize that we rely on the value in ints being
the same as the size in bytes.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:212: namespace {
Could we do this magic in BytecodeTraits instead?

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:280: for (int operand_index = 0; operand_index < i;
++operand_index) {
Could you add a TODO to calculate this at compile time if possible in the
future.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:475: return static_cast<OperandScale>(2 *
static_cast<int>(operand_scale));
Also please add a comment in OperandScale that we rely on their raw values

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

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:63: V(DebugBreak8, OperandType::kIdx,
OperandType::kIdx, OperandType::kFlag8)
I don't think you need as many of these any more do you, since we never insert
DebugBreak0 on scaled operands. I think we would just need:

V(DebugBreak0, OperandType::kNone)
V(DebugBreak1, OperandType::kReg)
V(DebugBreak2, OperandType::kReg, OperandType::kReg)
V(DebugBreak3, OperandType::kReg, OperandType::kReg, OperandType::kReg)
V(DebugBreak4, OperandType::kReg, OperandType::kReg, OperandType::kReg,
OperandType::kReg)
V(DebugBreak5, OperandType::kRuntimeId, OperandType::kReg, OperandType::kReg)

which should cover all non-scaled bytecodes.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:237: k1X = 1,
These names aren't very easy for me to parse. How about kSingle, kDouble and
kQuadruple?

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:254: V(None, false, false, OperandSize::kNone)      
        \
Could you add a comment describing what each of these parameters means

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:421: // prefix byte when scaling active.
I don't think this includes the prefix byte does it? Maybe just "... operands
for the given |operand_scale|"?

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:466: // Returns true if the bytecode is an scaling
prefix bytecode.
/s/an/a

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:129: bytecode_, operand_index,
operand_scale()))));
nit - could you pull out the IntPtrConstant as a variable to avoid the deep
nesting (and the same below).

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:231: Node* upper =
Can we pull out the shared code here and below in BytecodeOperandSignedQuad into
a helper function.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:594: Node* target_bytecode =
WordOr(target_bytecode_hi, target_bytecode_lo);
This seems pretty complicated, can't we just do:
int delta = operand_scale == OperandScale::k2X ? kBitsPerByte : 2 *
kBitsPerByte;
Node* new_bytecode_offset = Advance(Bytecodes::Size(bytecode_, operand_scale_);
Node* target_bytecode = Load(MachineType::Uint8(), BytecodeArrayTaggedPointer(),
new_bytecode_offet);
Node* target_bytecode = IntPtrAdd(target_bytecode, IntPtrConstant(delta));
...

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:35: OperandScale operand_scales[] =
{OperandScale::k1X, OperandScale::k2X,
const kOperandScales

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:37: for (size_t i = 0; i <
arraysize(operand_scales); ++i) {
nit - pull out:
OperandScale operand_scale = operand_scales[i];
and use it below.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:60: for (size_t index = 0; index <
arraysize(dispatch_table_); ++index) {
nit - comment that this is filling in empty dispatch entries with the illegal
bytecode handler.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:94: !Bytecodes::IsDebugBreak(bytecode));
Do you need the !Bytecodes::IsPrefixScalingBytecode(bytecode) &&
!Bytecodes::IsDebugBreak(bytecode) ? They shouldn't be counted as having
ScalableOperands should they?

https://codereview.chromium.org/1783483002/diff/430001/src/runtime/runtime-in...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/runtime/runtime-in...
src/runtime/runtime-interpreter.cc:140: offset >
bytecode_iterator.current_offset()) {
Should this be "if (offset == bytecode_iterator.current_offset())" as above?

https://codereview.chromium.org/1783483002/diff/430001/test/unittests/interpr...
File test/unittests/interpreter/constant-array-builder-unittest.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/test/unittests/interpr...
test/unittests/interpreter/constant-array-builder-unittest.cc:163:
TEST_F(ConstantArrayBuilderTest, ToLargeFixedArray) {
How long does this tests take to run? We previously had issues with the 16bit
varient, just want to make sure this doesn't regress test runtime again.

[oth, 2016-03-21 09:15:44]

Thanks, incorporated or reflected everything here. The one exception being
uint32_t for Register::ToOperand/FromOperand.

[ Had to delete the previous email in this reply as it exceed the 10k limit of
Reitveld :-) ]

[oth, 2016-03-21 09:16:54]

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

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:62: switch (operand_size) {
On 2016/03/17 17:30:49, rmcilroy wrote:
> I think you could use DecodeSignedOperand here to avoid duplicating code.

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:78: MUST_USE_RESULT int32_t
GetUnsignedOperand(int operand_index) const {
On 2016/03/17 17:30:49, rmcilroy wrote:
> should this return uint32_t instead?

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:92: DCHECK(operand_size >=
OperandSize::kByte &&
On 2016/03/17 17:30:49, rmcilroy wrote:
> Just DCHECK_NE(operand_size, OperandSize::kNone) should work

Function removed as part of common factoring of DecodeOperand.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:804: delta -= 1;
On 2016/03/17 17:30:49, rmcilroy wrote:
> nit - add a comment that this is compensating for the prefix bytecode which
will
> be emitted before the jump bytecode.

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1470: OperandScale operand_scale =
static_cast<OperandScale>(result);
On 2016/03/17 17:30:49, rmcilroy wrote:
> Could you add a STATIC_ASSERT that casting OperandSize to OperandScale gives
the
> expected results.

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1478: return
static_cast<uint32_t>(reg.ToOperand());
On 2016/03/17 17:30:49, rmcilroy wrote:
> Could we just make reg.ToOperand return a uint32_t and remove this method
(same
> for FromOperand below)?

Probably being overly conservative here but would prefer to leave as-is. We've
tagged operand types as signed or unsigned in this CL. Changing to uint32_t
would give us a type that needs the signed coercions, but is passed around as an
unsigned.

WDYT? It can be changed.

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

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:275: static OperandSize
SizeForRegisterOperand(Register reg);
On 2016/03/17 17:30:49, rmcilroy wrote:
> nit - newline above

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:279: static uint32_t
RegisterOperand(Register reg);
On 2016/03/17 17:30:49, rmcilroy wrote:
> nit - newline above. Also, can these all be private (search in this CL doesn't
> work too well, but I can't see uses outside BytecodeArrayBuilder)?

Done newline. Unittests check these work. The size is a killer, but getting
smaller all the time :-)

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:72: switch
(Bytecodes::SizeOfOperand(operand_type, current_operand_scale())) {
On 2016/03/17 17:30:49, rmcilroy wrote:
> I think you could use DecodeUnsignedOperand here to avoid duplicating code.

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:97: switch
(Bytecodes::SizeOfOperand(operand_type, current_operand_scale())) {
On 2016/03/17 17:30:49, rmcilroy wrote:
> I think you could use DecodeSignedOperand here to avoid duplicating code.

Yep, got all these before getting this far in the review :-)

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:124:
DCHECK(Bytecodes::GetOperandType(current_bytecode(), operand_index) ==
On 2016/03/17 17:30:49, rmcilroy wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:132: DCHECK(operand_type ==
OperandType::kIdx);
On 2016/03/17 17:30:49, rmcilroy wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.h (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.h:50: void update_operand_scale();
On 2016/03/17 17:30:50, rmcilroy wrote:
> nit - UpdateOperandScale  (not a getter or setter so should be CapsCase)

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:127: int delta = static_cast<int>(operand_size);
On 2016/03/17 17:30:50, rmcilroy wrote:
> Could you add a comment to OperandSize that we rely on the value in ints being
> the same as the size in bytes.

Done. And OperandScale.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:212: namespace {
On 2016/03/17 17:30:50, rmcilroy wrote:
> Could we do this magic in BytecodeTraits instead?

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:280: for (int operand_index = 0; operand_index < i;
++operand_index) {
On 2016/03/17 17:30:50, rmcilroy wrote:
> Could you add a TODO to calculate this at compile time if possible in the
> future.

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:475: return static_cast<OperandScale>(2 *
static_cast<int>(operand_scale));
On 2016/03/17 17:30:50, rmcilroy wrote:
> Also please add a comment in OperandScale that we rely on their raw values

Done.

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

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:63: V(DebugBreak8, OperandType::kIdx,
OperandType::kIdx, OperandType::kFlag8)
On 2016/03/17 17:30:50, rmcilroy wrote:
> I don't think you need as many of these any more do you, since we never insert
> DebugBreak0 on scaled operands. I think we would just need:
> 
> V(DebugBreak0, OperandType::kNone)
> V(DebugBreak1, OperandType::kReg)
> V(DebugBreak2, OperandType::kReg, OperandType::kReg)
> V(DebugBreak3, OperandType::kReg, OperandType::kReg, OperandType::kReg)
> V(DebugBreak4, OperandType::kReg, OperandType::kReg, OperandType::kReg,
> OperandType::kReg)
> V(DebugBreak5, OperandType::kRuntimeId, OperandType::kReg, OperandType::kReg)
> 
> which should cover all non-scaled bytecodes.

Good spot. We need both of the breaks with the kRuntimeId operand.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:237: k1X = 1,
On 2016/03/17 17:30:50, rmcilroy wrote:
> These names aren't very easy for me to parse. How about kSingle, kDouble and
> kQuadruple?

Done.

<Bikeshed>
It might be better generally if the naming between OperandSize and OperandScale
were more orthogonal. Specifically OperandSize being OneByte, TwoBytes,
FourBytes rather Byte, Short, Quad.
</Bikeshed>

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:254: V(None, false, false, OperandSize::kNone)      
        \
On 2016/03/17 17:30:50, rmcilroy wrote:
> Could you add a comment describing what each of these parameters means

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:421: // prefix byte when scaling active.
On 2016/03/17 17:30:50, rmcilroy wrote:
> I don't think this includes the prefix byte does it? Maybe just "... operands
> for the given |operand_scale|"?

Done. This was really stale, true many revisions back :-)

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/byteco...
src/interpreter/bytecodes.h:466: // Returns true if the bytecode is an scaling
prefix bytecode.
On 2016/03/17 17:30:50, rmcilroy wrote:
> /s/an/a

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:129: bytecode_, operand_index,
operand_scale()))));
On 2016/03/17 17:30:50, rmcilroy wrote:
> nit - could you pull out the IntPtrConstant as a variable to avoid the deep
> nesting (and the same below).

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:231: Node* upper =
On 2016/03/17 17:30:50, rmcilroy wrote:
> Can we pull out the shared code here and below in BytecodeOperandSignedQuad
into
> a helper function.

The latest revision tries to make more of the unaligned reads common, reducing
the Quad and Short reads to use a shared helper. WDYT?

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:594: Node* target_bytecode =
WordOr(target_bytecode_hi, target_bytecode_lo);
On 2016/03/17 17:30:50, rmcilroy wrote:
> This seems pretty complicated, can't we just do:
> int delta = operand_scale == OperandScale::k2X ? kBitsPerByte : 2 *
> kBitsPerByte;
> Node* new_bytecode_offset = Advance(Bytecodes::Size(bytecode_,
operand_scale_);
> Node* target_bytecode = Load(MachineType::Uint8(),
BytecodeArrayTaggedPointer(),
> new_bytecode_offet);
> Node* target_bytecode = IntPtrAdd(target_bytecode, IntPtrConstant(delta));
> ...

Okay, have reworked to a middle ground and can a clarifying comment.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:35: OperandScale operand_scales[] =
{OperandScale::k1X, OperandScale::k2X,
On 2016/03/17 17:30:50, rmcilroy wrote:
> const kOperandScales

Meant to remove this and replace with NextOperandScale. Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:37: for (size_t i = 0; i <
arraysize(operand_scales); ++i) {
On 2016/03/17 17:30:50, rmcilroy wrote:
> nit - pull out:
> OperandScale operand_scale = operand_scales[i];
> and use it below.

operand_scale is now the loop variable.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:60: for (size_t index = 0; index <
arraysize(dispatch_table_); ++index) {
On 2016/03/17 17:30:50, rmcilroy wrote:
> nit - comment that this is filling in empty dispatch entries with the illegal
> bytecode handler.

Done.

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter.cc:94: !Bytecodes::IsDebugBreak(bytecode));
On 2016/03/17 17:30:50, rmcilroy wrote:
> Do you need the !Bytecodes::IsPrefixScalingBytecode(bytecode) &&
> !Bytecodes::IsDebugBreak(bytecode) ? They shouldn't be counted as having
> ScalableOperands should they?

Not with the reduced set of debug breaks and less paranoia.

https://codereview.chromium.org/1783483002/diff/430001/src/runtime/runtime-in...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/runtime/runtime-in...
src/runtime/runtime-interpreter.cc:140: offset >
bytecode_iterator.current_offset()) {
On 2016/03/17 17:30:50, rmcilroy wrote:
> Should this be "if (offset == bytecode_iterator.current_offset())" as above?

A comment has been added to the code to clarify.

The bytecode_iterator.current_offset() refers to the start position of the
prefix bytecode if present. The iterator visits the the prefix and widend
bytecode as a group. Printing the registers is suppressed until the widend
bytecode has exited (offset > current_offset && operand_scale > 1).

https://codereview.chromium.org/1783483002/diff/430001/test/unittests/interpr...
File test/unittests/interpreter/constant-array-builder-unittest.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/test/unittests/interpr...
test/unittests/interpreter/constant-array-builder-unittest.cc:163:
TEST_F(ConstantArrayBuilderTest, ToLargeFixedArray) {
On 2016/03/17 17:30:50, rmcilroy wrote:
> How long does this tests take to run? We previously had issues with the 16bit
> varient, just want to make sure this doesn't regress test runtime again.

Good point. I did check but think we'd need to test this either way as there was
a mistake that this catches.

Debug.x64 on MBA 2.13 GHz reports 34-38ms.

[Yang, 2016-03-21 09:36:13]

https://codereview.chromium.org/1783483002/diff/490001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1783483002/diff/490001/src/debug/debug.cc#new...
src/debug/debug.cc:1583: bytecode, interpreter::OperandScale::kSingle);
Is the operand scale guaranteed to be single here?

What we do here is to load the original bytecode from the original bytecode
array to continue execution as if the original bytecode was there. If the
original bytecode had a different operand scale, this would be wrong?

[oth, 2016-03-21 09:52:14]

Hi Yang, thanks for looking at this.

https://codereview.chromium.org/1783483002/diff/490001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1783483002/diff/490001/src/debug/debug.cc#new...
src/debug/debug.cc:1583: bytecode, interpreter::OperandScale::kSingle);
On 2016/03/21 09:36:13, Yang wrote:
> Is the operand scale guaranteed to be single here?
> 
> What we do here is to load the original bytecode from the original bytecode
> array to continue execution as if the original bytecode was there. If the
> original bytecode had a different operand scale, this would be wrong?

If the bytecode has a scaling prefix, then the scaling prefix has the debug
break on it. There are special debug versions of the prefix bytecodes
(DEBUG_BREAK_PREFIX_BYTECODE_LIST). Otherwise, it's the usual mechanism.

[oth, 2016-03-21 09:53:13]

On 2016/03/21 09:52:14, oth wrote:
> Hi Yang, thanks for looking at this.
> 
> https://codereview.chromium.org/1783483002/diff/490001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/1783483002/diff/490001/src/debug/debug.cc#new...
> src/debug/debug.cc:1583: bytecode, interpreter::OperandScale::kSingle);
> On 2016/03/21 09:36:13, Yang wrote:
> > Is the operand scale guaranteed to be single here?
> > 
> > What we do here is to load the original bytecode from the original bytecode
> > array to continue execution as if the original bytecode was there. If the
> > original bytecode had a different operand scale, this would be wrong?
> 
> If the bytecode has a scaling prefix, then the scaling prefix has the debug
> break on it. There are special debug versions of the prefix bytecodes
> (DEBUG_BREAK_PREFIX_BYTECODE_LIST). Otherwise, it's the usual mechanism.

BTW, there's also a test that goes with this:
https://codereview.chromium.org/1783483002/diff/490001/test/mjsunit/ignition/...

Thanks

[Yang, 2016-03-21 10:00:27]

I see. LGTM in that case. Thanks.

[Michael Starzinger, 2016-03-21 10:11:43]

LGTM on compiler. I like it. Thanks!

[rmcilroy, 2016-03-21 12:41:36]

A final couple of comments, but LGTM, thanks!

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:231: Node* upper =
On 2016/03/21 09:16:54, oth wrote:
> On 2016/03/17 17:30:50, rmcilroy wrote:
> > Can we pull out the shared code here and below in BytecodeOperandSignedQuad
> into
> > a helper function.
> 
> The latest revision tries to make more of the unaligned reads common, reducing
> the Quad and Short reads to use a shared helper. WDYT?

Nice, looks good - couple of comments up above. Also, did you test this out on
mips / ppc?

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:594: Node* target_bytecode =
WordOr(target_bytecode_hi, target_bytecode_lo);
On 2016/03/21 09:16:54, oth wrote:
> On 2016/03/17 17:30:50, rmcilroy wrote:
> > This seems pretty complicated, can't we just do:
> > int delta = operand_scale == OperandScale::k2X ? kBitsPerByte : 2 *
> > kBitsPerByte;
> > Node* new_bytecode_offset = Advance(Bytecodes::Size(bytecode_,
> operand_scale_);
> > Node* target_bytecode = Load(MachineType::Uint8(),
> BytecodeArrayTaggedPointer(),
> > new_bytecode_offet);
> > Node* target_bytecode = IntPtrAdd(target_bytecode, IntPtrConstant(delta));
> > ...
> 
> Okay, have reworked to a middle ground and can a clarifying comment.

Looks good, thanks!

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

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1451: // Operand sizes have been
scaled and currently all byte sized at scale 1X.
Update comment (OperandScale::kSingle)

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:151: compiler::Node*
InterpreterAssembler::BytecodeReadUnalignedBytes(
nit - BytecodeOperandReadUnaligned(...)

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:152: int relative_offset, int count,
MachineType msb_type) {
Could we just pass the MachineType of the end result in here (nit - as the first
argument) then you can avoid the count argument (just derive it from the output
type) and msb_type (again derive it from whether the output type is signed or
unsigned).

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:153: static const int kMaxCount = 4;
DCHECK(!TargetSupportsUnalignedAccess())

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:186: // unsigned.
Mention this should only be used on architectures which don't explicitly support
unaligned access natively.

https://codereview.chromium.org/1783483002/diff/450001/src/runtime/runtime-in...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/450001/src/runtime/runtime-in...
src/runtime/runtime-interpreter.cc:139: // (potentially) widend bytecode has
completed. The iterator reports
/s/widend/widened/

[oth, 2016-03-21 14:21:49]

Thanks, all incorporated. Let's land this sucker :-)

                  _-*
          _------'
        _/
       / /
      / /
     / /
    / /
   / /
  (_)

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/430001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:231: Node* upper =
On 2016/03/21 12:41:36, rmcilroy wrote:
> On 2016/03/21 09:16:54, oth wrote:
> > On 2016/03/17 17:30:50, rmcilroy wrote:
> > > Can we pull out the shared code here and below in
BytecodeOperandSignedQuad
> > into
> > > a helper function.
> > 
> > The latest revision tries to make more of the unaligned reads common,
reducing
> > the Quad and Short reads to use a shared helper. WDYT?
> 
> Nice, looks good - couple of comments up above. Also, did you test this out on
> mips / ppc?

Ack. MIPS looks okay. IIRC PPC ran earlier, but build is broken on ToT so having
rebased, life is not good.

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

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:1451: // Operand sizes have been
scaled and currently all byte sized at scale 1X.
On 2016/03/21 12:41:36, rmcilroy wrote:
> Update comment (OperandScale::kSingle)

Done.

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:151: compiler::Node*
InterpreterAssembler::BytecodeReadUnalignedBytes(
On 2016/03/21 12:41:36, rmcilroy wrote:
> nit - BytecodeOperandReadUnaligned(...)

Done.

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:152: int relative_offset, int count,
MachineType msb_type) {
On 2016/03/21 12:41:36, rmcilroy wrote:
> Could we just pass the MachineType of the end result in here (nit - as the
first
> argument) then you can avoid the count argument (just derive it from the
output
> type) and msb_type (again derive it from whether the output type is signed or
> unsigned).

Done.

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:153: static const int kMaxCount = 4;
On 2016/03/21 12:41:36, rmcilroy wrote:
> DCHECK(!TargetSupportsUnalignedAccess())

Done.

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1783483002/diff/450001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:186: // unsigned.
On 2016/03/21 12:41:36, rmcilroy wrote:
> Mention this should only be used on architectures which don't explicitly
support
> unaligned access natively.

Done.

https://codereview.chromium.org/1783483002/diff/450001/src/runtime/runtime-in...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1783483002/diff/450001/src/runtime/runtime-in...
src/runtime/runtime-interpreter.cc:139: // (potentially) widend bytecode has
completed. The iterator reports
On 2016/03/21 12:41:36, rmcilroy wrote:
> /s/widend/widened/

Done.

[oth, 2016-03-21 14:44:13]

The CQ bit was checked by oth@chromium.org

[oth, 2016-03-21 14:44:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1783483002/#ps550001
(title: "Re-generate golden files.")

[commit-bot: I haz the power, 2016-03-21 14:44:21]

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

[commit-bot: I haz the power, 2016-03-21 17:08:27]

Description was changed from

==========
[interpreter] Add support for scalable operands.

This change introduces wide prefix bytecodes to support wide (16-bit)
and extra-wide (32-bit) operands. It retires the previous
wide-bytecodes and reduces the number of operand types.

Operands are now either scalable or fixed size. Scalable operands
increase in width when a bytecode is prefixed with wide or extra-wide.

The bytecode handler table is extended to 256*3 entries. The
first 256 entries are used for bytecodes with 8-bit operands,
the second 256 entries are used for bytecodes with operands that
scale to 16-bits, and the third group of 256 entries are used for
bytecodes with operands that scale to 32-bits. 

LOG=N
BUG=v8:4747,v8:4280
==========

to

==========
[interpreter] Add support for scalable operands.

This change introduces wide prefix bytecodes to support wide (16-bit)
and extra-wide (32-bit) operands. It retires the previous
wide-bytecodes and reduces the number of operand types.

Operands are now either scalable or fixed size. Scalable operands
increase in width when a bytecode is prefixed with wide or extra-wide.

The bytecode handler table is extended to 256*3 entries. The
first 256 entries are used for bytecodes with 8-bit operands,
the second 256 entries are used for bytecodes with operands that
scale to 16-bits, and the third group of 256 entries are used for
bytecodes with operands that scale to 32-bits. 

LOG=N
BUG=v8:4747,v8:4280
==========

[commit-bot: I haz the power, 2016-03-21 17:08:29]

Committed patchset #29 (id:550001)

[commit-bot: I haz the power, 2016-03-21 17:09:53]

Description was changed from

==========
[interpreter] Add support for scalable operands.

This change introduces wide prefix bytecodes to support wide (16-bit)
and extra-wide (32-bit) operands. It retires the previous
wide-bytecodes and reduces the number of operand types.

Operands are now either scalable or fixed size. Scalable operands
increase in width when a bytecode is prefixed with wide or extra-wide.

The bytecode handler table is extended to 256*3 entries. The
first 256 entries are used for bytecodes with 8-bit operands,
the second 256 entries are used for bytecodes with operands that
scale to 16-bits, and the third group of 256 entries are used for
bytecodes with operands that scale to 32-bits. 

LOG=N
BUG=v8:4747,v8:4280
==========

to

==========
[interpreter] Add support for scalable operands.

This change introduces wide prefix bytecodes to support wide (16-bit)
and extra-wide (32-bit) operands. It retires the previous
wide-bytecodes and reduces the number of operand types.

Operands are now either scalable or fixed size. Scalable operands
increase in width when a bytecode is prefixed with wide or extra-wide.

The bytecode handler table is extended to 256*3 entries. The
first 256 entries are used for bytecodes with 8-bit operands,
the second 256 entries are used for bytecodes with operands that
scale to 16-bits, and the third group of 256 entries are used for
bytecodes with operands that scale to 32-bits.

LOG=N
BUG=v8:4747,v8:4280

Committed: https://crrev.com/48d082af38f692a27b4fb8caf28dbb426d69424a
Cr-Commit-Position: refs/heads/master@{#34955}
==========

[commit-bot: I haz the power, 2016-03-21 17:09:55]

Patchset 29 (id:??) landed as
https://crrev.com/48d082af38f692a27b4fb8caf28dbb426d69424a
Cr-Commit-Position: refs/heads/master@{#34955}

[Michael Achenbach, 2016-03-22 09:59:30]

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

[Michael Achenbach, 2016-03-22 09:59:31]

Please blacklist/fix the msan failure to unblock rolling:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20...