[Interpreter] Add support for jumps using constants with wide operands.

[Interpreter] Add support for jumps using constants with wide operands.

This increases the size of addressable constant pool entries for jumps
to match other bytecodes using operands indexing the constant pool.

This change also introduces reservations for constant pool entries.
Reservations are used for forward jumps to ensure a constant pool entry
will be available when the jump target (label) is bound and the jump is
patched up in the bytecode array.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/8109f63fd57317fef94c18824ee6ac187d9f5292
Cr-Commit-Position: refs/heads/master@{#33125}

[oth, 2015-12-23 12:30:42]

Description was changed from

==========
[Interpreter] Add support for jumps using constants with wide operands.

This change also reserves constant pool entries for forward jumps to ensure
a constant pool entry will be available when forward jump is patched. Forward
jumps are patched when the label is bound. Reservations may be for constant
pool entries with 8-bit or 16-bit indicies.

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

to

==========
[Interpreter] Add support for jumps using constants with wide operands.

This increases the size of addressable constant pool entries for
jumps to match other bytecodes using operands indexing the constant pool.

This change also introduces reservations for constant pool entries for
forward jumps to ensure an entry will be available when forward jump
is patched (when the associated label is bound).

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

[oth, 2015-12-23 12:33:55]

Description was changed from

==========
[Interpreter] Add support for jumps using constants with wide operands.

This increases the size of addressable constant pool entries for
jumps to match other bytecodes using operands indexing the constant pool.

This change also introduces reservations for constant pool entries for
forward jumps to ensure an entry will be available when forward jump
is patched (when the associated label is bound).

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

to

==========
[Interpreter] Add support for jumps using constants with wide operands.

This increases the size of addressable constant pool entries for
jumps to match other bytecodes using operands indexing the constant pool.

This change also introduces reservations for constant pool entries.
Reservations are used for forward jumps to ensure a constant pool
entry will be available when the jump target (label) is bound and the
jump is patched up in the bytecode array.

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

[oth, 2015-12-23 12:36:21]

Description was changed from

==========
[Interpreter] Add support for jumps using constants with wide operands.

This increases the size of addressable constant pool entries for
jumps to match other bytecodes using operands indexing the constant pool.

This change also introduces reservations for constant pool entries.
Reservations are used for forward jumps to ensure a constant pool
entry will be available when the jump target (label) is bound and the
jump is patched up in the bytecode array.

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

to

==========
[Interpreter] Add support for jumps using constants with wide operands.         
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This increases the size of addressable constant pool entries for jumps          
                                                                                
                                                                                
                                                                            
to match other bytecodes using operands indexing the constant pool.             
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This change also introduces reservations for constant pool entries.             
                                                                                
                                                                                
                                                                            
Reservations are used for forward jumps to ensure a constant pool entry         
                                                                                
                                                                                
                                                                            
will be available when the jump target (label) is bound and the jump is         
                                                                                
                                                                                
                                                                            
patched up in the bytecode array.

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

[oth, 2015-12-23 12:44:01]

oth@chromium.org changed reviewers:
+ bmeurer@chromium.org, mythria@chromium.org, rmicilroy@chromium.org

[oth, 2015-12-23 12:44:02]

Benedikt, could you please take a look at the code and tests under compiler.
Minimal changes that build on existing code paths.

Mythri, Ross, PTAL.

[mythria, 2015-12-24 15:53:44]

looks great. I have very minor comments. 
May be we could try /mjsunit/switch.js. This was failing because of patchJump
could not find a constant pool entry. 
The following is the command which caused it. 

/usr/local/google/code/v8/out/x64.debug/d8 --test --random-seed=-648783724
--ignition --ignition-filter=* --ignition-fake-try-catch
--ignition-fallback-on-eval-and-catch --nohard-abort --nodead-code-elimination
--nofold-constants --enable-slow-asserts --debug-code --verify-heap
--allow-natives-syntax /usr/local/google/code/v8/test/mjsunit/mjsunit.js
/usr/local/google/code/v8/test/mjsunit/switch.js

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

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:801: if
(Bytecodes::IsJumpImmediate(jump_bytecode)) {
May be we can check for reservation token to be Idx8 instead of jump_bytecode? I
thought it will be easier to follow.

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
File src/interpreter/constant-array-builder.cc (right):

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:59: EnsureCapacity(allocated_low_ +
1);
I might have missed something, but would the following work here?
constants_.push_back(object); 
allocated_low_++;

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:106: // in the low range to satisfy
the commitment.
Do we need constants_[allocated_low_] = object; here?

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
File src/interpreter/constant-array-builder.h (right):

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
src/interpreter/constant-array-builder.h:56: static size_t
SizeOfReservedArrayIndex(ReservationToken token);
I am not sure we use this function.

[Benedikt Meurer, 2015-12-27 07:38:04]

LGTM on compiler.

[oth, 2015-12-27 08:42:34]

Thanks both for looking at this.

I'm going to write a different implementation of the constant-array-builder to
see if it'll simplify the logic.

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

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:801: if
(Bytecodes::IsJumpImmediate(jump_bytecode)) {
On 2015/12/24 15:53:44, mythria wrote:
> May be we can check for reservation token to be Idx8 instead of jump_bytecode?
I
> thought it will be easier to follow. 

Done.

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
File src/interpreter/constant-array-builder.cc (right):

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:59: EnsureCapacity(allocated_low_ +
1);
On 2015/12/24 15:53:44, mythria wrote:
> I might have missed something, but would the following work here?
> constants_.push_back(object); 
> allocated_low_++; 
> 

Consider the case where a reservation has been made in the low range
and a subsequent allocation goes into the high range. The constants
array has a gap for the reserved entries and must be big enough for the
entries in the high range too. Then code discards a low reservation
and subsequently allocates. The new entry needs to be inserted into
the hole in the low entries. If it's pushed back it'll be at the end of the
high entries.

I am thinking this code might be easier to reasonable about if there were an
internal ConstantsArrayRange class that held a portion of the constants_ array
and reservations for the low and high ranges. There'd be less special casing.

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:106: // in the low range to satisfy
the commitment.
On 2015/12/24 15:53:44, mythria wrote:
> Do we need constants_[allocated_low_] = object; here?

Yes.

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
File src/interpreter/constant-array-builder.h (right):

https://codereview.chromium.org/1546683002/diff/80001/src/interpreter/constan...
src/interpreter/constant-array-builder.h:56: static size_t
SizeOfReservedArrayIndex(ReservationToken token);
On 2015/12/24 15:53:44, mythria wrote:
> I am not sure we use this function. 

Thanks, removed.

[oth, 2016-01-04 10:48:27]

On 2015/12/24 15:53:44, mythria wrote:
> looks great. I have very minor comments. 
> May be we could try /mjsunit/switch.js. This was failing because of patchJump
> could not find a constant pool entry. 
> The following is the command which caused it. 
> 
> /usr/local/google/code/v8/out/x64.debug/d8 --test --random-seed=-648783724
> --ignition --ignition-filter=* --ignition-fake-try-catch
> --ignition-fallback-on-eval-and-catch --nohard-abort --nodead-code-elimination
> --nofold-constants --enable-slow-asserts --debug-code --verify-heap
> --allow-natives-syntax /usr/local/google/code/v8/test/mjsunit/mjsunit.js
> /usr/local/google/code/v8/test/mjsunit/switch.js

Thanks for flagging this. The test passes with this change and the test is
enabled in patch set 10. This seems to be the only existing test failure we had
with this.

Orion

[mythria, 2016-01-04 14:10:33]

lgtm

[rmcilroy, 2016-01-04 17:05:47]

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

[rmcilroy, 2016-01-04 17:05:48]

Looks good overall. I've not looked at tests yet though.

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

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:774: return
Bytecode::kJumpConstantWide;
nit - return static_cast<Bytecode>(-1) (and above)

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:802: auto operand_location =
jump_location + 1;
Don't use auto here (it's not immediately obvious what the type is).

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:804:
static_cast<ConstantArrayBuilder::ReservationToken>(*operand_location);
Could we wrap up this ReservationToken getter stuff in
ConstantArrayBuilder::GetToken(const ZoneVector<uint8_t>::iterator&
jump_location)

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:805: if
(ConstantArrayBuilder::ReservationToken::kIdx8 == reservation_token) {
nit - switch on ReservationToken type

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:876: auto token =
constant_array_builder()->CreateReservedEntry();
don't use auto here (not immediately clear what the type is).

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:877: switch (token) {
Please add a comment mentioning what you are doing here (storing the token in
the operand).

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:884: target |= target << 8;
nit - I assume this is for big endian support? Please add a comment

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

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:15: int width)
The width thing is a bit complex - I think it would be simpler just to have a
start_index and end_index, WDYT ?

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:29: size_t
ConstantArrayBuilder::ConstantArraySlice::free() const {
free is a bit of an overloaded name. How about remaining().

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:76: STATIC_CONST_MEMBER_DEFINITION
const size_t ConstantArrayBuilder::kLowCapacity;
Do you need these? I didn't think you needed them unless you took the address of
the static const?

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:165: if (token ==
ReservationToken::kIdx8 && *entry >= idx8_slice_.capacity()) {
Could the opposite happen (token == kIdx16 but we got an entry in kIdx8)? I
suppose this doesn't matter if it does since the wide variant will still work?

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
File src/interpreter/constant-array-builder.h (right):

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:23: static const size_t kMaxCapacity =
65536;
nit - newline above

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:23: static const size_t kMaxCapacity =
65536;
/s/65536/kMaxUInt16 + 1/

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:24: static const size_t kLowCapacity =
256;
kMaxUInt8 + 1

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:30: PretenureFlag pretenure) const;
no need for PretenureFlag argument - this should always be TENURED

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:68: void unreserve();
lower case only for getters / setters (which you could probably do inline here).
reserve, unreserve, allocate (and possibly at) should be upper case function
names.

[oth, 2016-01-05 09:30:08]

Thanks for the ever excellent comments, all incorporated. Also dropped the
ConstantArrayBuilder::ReservationToken and now just use OperandSize which cleans
up a lot of wordiness.

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

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:774: return
Bytecode::kJumpConstantWide;
On 2016/01/04 17:05:48, rmcilroy wrote:
> nit - return static_cast<Bytecode>(-1) (and above)

Done.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:802: auto operand_location =
jump_location + 1;
On 2016/01/04 17:05:48, rmcilroy wrote:
> Don't use auto here (it's not immediately obvious what the type is).

Done.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:804:
static_cast<ConstantArrayBuilder::ReservationToken>(*operand_location);
On 2016/01/04 17:05:48, rmcilroy wrote:
> Could we wrap up this ReservationToken getter stuff in
> ConstantArrayBuilder::GetToken(const ZoneVector<uint8_t>::iterator&
> jump_location)

Simplified the code path.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:805: if
(ConstantArrayBuilder::ReservationToken::kIdx8 == reservation_token) {
On 2016/01/04 17:05:48, rmcilroy wrote:
> nit - switch on ReservationToken type

Done. Moved the switch bodies into separate functions.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:876: auto token =
constant_array_builder()->CreateReservedEntry();
On 2016/01/04 17:05:48, rmcilroy wrote:
> don't use auto here (not immediately clear what the type is).

Done.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:877: switch (token) {
On 2016/01/04 17:05:48, rmcilroy wrote:
> Please add a comment mentioning what you are doing here (storing the token in
> the operand).

Removed. The token was supposed to be symbolic - make a reservation, get a
token, cash it in. However, there are only two token values and on reflection I
don't think it serves any practical purpose.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:884: target |= target << 8;
On 2016/01/04 17:05:48, rmcilroy wrote:
> nit - I assume this is for big endian support? Please add a comment

Yes. No longer exists.

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

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:15: int width)
On 2016/01/04 17:05:48, rmcilroy wrote:
> The width thing is a bit complex - I think it would be simpler just to have a
> start_index and end_index, WDYT ?

Yes, this shouldn't have been left in.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:29: size_t
ConstantArrayBuilder::ConstantArraySlice::free() const {
On 2016/01/04 17:05:48, rmcilroy wrote:
> free is a bit of an overloaded name. How about remaining().

Glass half-full? available() :-)

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:76: STATIC_CONST_MEMBER_DEFINITION
const size_t ConstantArrayBuilder::kLowCapacity;
On 2016/01/04 17:05:48, rmcilroy wrote:
> Do you need these? I didn't think you needed them unless you took the address
of
> the static const?

Yes, both external linkage and taking the constant's address requires these. The
unittests for ConstantArrayBuilder use this.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:165: if (token ==
ReservationToken::kIdx8 && *entry >= idx8_slice_.capacity()) {
On 2016/01/04 17:05:48, rmcilroy wrote:
> Could the opposite happen (token == kIdx16 but we got an entry in kIdx8)? I
> suppose this doesn't matter if it does since the wide variant will still work?

If the opposite happens it doesn't matter. The bytecode stream has space for a
2-byte operand. The emitted bytecode will be for a 2-byte operand and it get's
appropriately zero padded. Any subsequent duplicate object additions will get
the 1-byte operand index.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
File src/interpreter/constant-array-builder.h (right):

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:23: static const size_t kMaxCapacity =
65536;
On 2016/01/04 17:05:48, rmcilroy wrote:
> nit - newline above

Done.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:23: static const size_t kMaxCapacity =
65536;
On 2016/01/04 17:05:48, rmcilroy wrote:
> /s/65536/kMaxUInt16 + 1/

Done.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:24: static const size_t kLowCapacity =
256;
On 2016/01/04 17:05:48, rmcilroy wrote:
> kMaxUInt8 + 1

Done.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:30: PretenureFlag pretenure) const;
On 2016/01/04 17:05:48, rmcilroy wrote:
> no need for PretenureFlag argument - this should always be TENURED

Done.

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:68: void unreserve();
On 2016/01/04 17:05:48, rmcilroy wrote:
> lower case only for getters / setters (which you could probably do inline
here).
> reserve, unreserve, allocate (and possibly at) should be upper case function
> names.

Done.

[rmcilroy, 2016-01-05 13:46:10]

LGTM with some final comments.

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

https://codereview.chromium.org/1546683002/diff/200001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:29: size_t
ConstantArrayBuilder::ConstantArraySlice::free() const {
On 2016/01/05 09:30:08, oth wrote:
> On 2016/01/04 17:05:48, rmcilroy wrote:
> > free is a bit of an overloaded name. How about remaining().
> 
> Glass half-full? available() :-)

SGTM :).

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

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:806:
constant_array_builder()->DiscardReservedEntry(OperandSize::kByte);
Could you add a DCHECK that the bytecode is the expected non-constant pool type
jump.

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:841: Bytecode jump_bytecode =
Bytecodes::FromByte(*jump_location);
nit - you could calculate delta here and pass it to the PatchIndirect..
functions instead of jump_target.

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

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:138: } else {
We should probably add a (non-debug) CHECK that there is capacity in the
idx16_slice to ensure we crash rather than causing a buffer overrun.

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:151: } else {
Also a CHECK here (or an else if idx8_slice_.available() > 0) { ... } else {
UNREACHABLE(); }?

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
File src/interpreter/constant-array-builder.h (right):

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:72: size_t start_index() const;
nit - accessors are usually below the other functions.

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:2114: int start =
isolate->random_number_generator()->NextInt(kStep);
Do you really need this random number generator? I think all it would bring
would be potential flakeyness (assuming the tests don't have a fixed seed) or
would be fixed anyway (assuming the tests use a fixed seed).

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:2123: os.str("");
Please use a different std::stringstream  rather than resetting the one above

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:2693: "    W = 'W5', X =
'X5', Y = 'Y5', Z = 'Z5';\n"
Optional suggestion - if you just want to fill up the constant pool then the
following will do it:
  REPEAT_256(SPACE, "var y = 2.3;")

Which can be matched below as: 
  REPEAT_256(COMMA,                                  //
             B(LdaConstant), U8(const_count++),     //
             B(Star), R(0)),                         //

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3204: int start =
isolate->random_number_generator()->NextInt(kStep);
Ditto regarding random number generator.

[oth, 2016-01-05 18:31:59]

Thanks, all incorporated.

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

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:806:
constant_array_builder()->DiscardReservedEntry(OperandSize::kByte);
On 2016/01/05 13:46:10, rmcilroy wrote:
> Could you add a DCHECK that the bytecode is the expected non-constant pool
type
> jump.

Done + added DCHECK that the operand is zero in both the 8-bit and 16-bit patch
routines.

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:841: Bytecode jump_bytecode =
Bytecodes::FromByte(*jump_location);
On 2016/01/05 13:46:10, rmcilroy wrote:
> nit - you could calculate delta here and pass it to the PatchIndirect..
> functions instead of jump_target.

Done.

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

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:138: } else {
On 2016/01/05 13:46:10, rmcilroy wrote:
> We should probably add a (non-debug) CHECK that there is capacity in the
> idx16_slice to ensure we crash rather than causing a buffer overrun.

Done.

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
src/interpreter/constant-array-builder.cc:151: } else {
On 2016/01/05 13:46:10, rmcilroy wrote:
> Also a CHECK here (or an else if idx8_slice_.available() > 0) { ... } else {
> UNREACHABLE(); }?

Done.

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
File src/interpreter/constant-array-builder.h (right):

https://codereview.chromium.org/1546683002/diff/240001/src/interpreter/consta...
src/interpreter/constant-array-builder.h:72: size_t start_index() const;
On 2016/01/05 13:46:10, rmcilroy wrote:
> nit - accessors are usually below the other functions.

Done.

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:2114: int start =
isolate->random_number_generator()->NextInt(kStep);
On 2016/01/05 13:46:10, rmcilroy wrote:
> Do you really need this random number generator? I think all it would bring
> would be potential flakeyness (assuming the tests don't have a fixed seed) or
> would be fixed anyway (assuming the tests use a fixed seed).

Done. The hope was the bots ran different seeds and this'd drive more coverage.
Considered capturing the seed here, but this would still make repro awkward.

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:2123: os.str("");
On 2016/01/05 13:46:10, rmcilroy wrote:
> Please use a different std::stringstream  rather than resetting the one above

Done.

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
test/cctest/interpreter/test-bytecode-generator.cc:2693: "    W = 'W5', X =
'X5', Y = 'Y5', Z = 'Z5';\n"
On 2016/01/05 13:46:10, rmcilroy wrote:
> Optional suggestion - if you just want to fill up the constant pool then the
> following will do it:
>   REPEAT_256(SPACE, "var y = 2.3;")
> 
> Which can be matched below as: 
>   REPEAT_256(COMMA,                                  //
>              B(LdaConstant), U8(const_count++),     //
>              B(Star), R(0)),                         //

Done. There are 16-bit operands U16 which means doing ++ increases by 2 for
those elements, not insurmountable.

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1546683002/diff/240001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:3204: int start =
isolate->random_number_generator()->NextInt(kStep);
On 2016/01/05 13:46:10, rmcilroy wrote:
> Ditto regarding random number generator.

Done. Plus removed the double use of stringstream here too.

[oth, 2016-01-05 18:38:39]

The CQ bit was checked by oth@chromium.org

[oth, 2016-01-05 18:38:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mythria@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1546683002/#ps300001
(title: "Incorporate comments on patchset 13.")

[commit-bot: I haz the power, 2016-01-05 18:38:44]

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

[commit-bot: I haz the power, 2016-01-05 19:08:17]

Description was changed from

==========
[Interpreter] Add support for jumps using constants with wide operands.         
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This increases the size of addressable constant pool entries for jumps          
                                                                                
                                                                                
                                                                            
to match other bytecodes using operands indexing the constant pool.             
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This change also introduces reservations for constant pool entries.             
                                                                                
                                                                                
                                                                            
Reservations are used for forward jumps to ensure a constant pool entry         
                                                                                
                                                                                
                                                                            
will be available when the jump target (label) is bound and the jump is         
                                                                                
                                                                                
                                                                            
patched up in the bytecode array.

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

to

==========
[Interpreter] Add support for jumps using constants with wide operands.         
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This increases the size of addressable constant pool entries for jumps          
                                                                                
                                                                                
                                                                            
to match other bytecodes using operands indexing the constant pool.             
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This change also introduces reservations for constant pool entries.             
                                                                                
                                                                                
                                                                            
Reservations are used for forward jumps to ensure a constant pool entry         
                                                                                
                                                                                
                                                                            
will be available when the jump target (label) is bound and the jump is         
                                                                                
                                                                                
                                                                            
patched up in the bytecode array.

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

[commit-bot: I haz the power, 2016-01-05 19:08:18]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-01-05 19:09:20]

Description was changed from

==========
[Interpreter] Add support for jumps using constants with wide operands.         
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This increases the size of addressable constant pool entries for jumps          
                                                                                
                                                                                
                                                                            
to match other bytecodes using operands indexing the constant pool.             
                                                                                
                                                                                
                                                                            
                                                                                
                                                                                
                                                                                
                                                                            
This change also introduces reservations for constant pool entries.             
                                                                                
                                                                                
                                                                            
Reservations are used for forward jumps to ensure a constant pool entry         
                                                                                
                                                                                
                                                                            
will be available when the jump target (label) is bound and the jump is         
                                                                                
                                                                                
                                                                            
patched up in the bytecode array.

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

to

==========
[Interpreter] Add support for jumps using constants with wide operands.

This increases the size of addressable constant pool entries for jumps
to match other bytecodes using operands indexing the constant pool.

This change also introduces reservations for constant pool entries.
Reservations are used for forward jumps to ensure a constant pool entry
will be available when the jump target (label) is bound and the jump is
patched up in the bytecode array.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/8109f63fd57317fef94c18824ee6ac187d9f5292
Cr-Commit-Position: refs/heads/master@{#33125}
==========

[commit-bot: I haz the power, 2016-01-05 19:09:21]

Patchset 16 (id:??) landed as
https://crrev.com/8109f63fd57317fef94c18824ee6ac187d9f5292
Cr-Commit-Position: refs/heads/master@{#33125}