[interpreter] Preparation for 32-bit operands.

[interpreter] Preparation for 32-bit operands.

Extends the constant pool to deal with more slices.

Adds ReadUnalignedUInt32().

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

Committed: https://crrev.com/1db484f7d8b27405af489b23252b8187c51608dc
Cr-Commit-Position: refs/heads/master@{#34319}

[oth, 2016-02-25 12:19:41]

Michi, could you look at the change in utils.h.

Ross, could you look over the change to constant-array-builder. It is missing
the slice for 32-bit operands, but planning to introduce that in the operand CL.

[rmcilroy, 2016-02-25 13:26:05]

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

[rmcilroy, 2016-02-25 13:26:06]

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

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:74:
ConstantArrayBuilder::ConstantArraySlice* ConstantArrayBuilder::IndexToSlice(
This seems a bit overkill to me. I'd prefer just to do the following in
ConstantArrayBuilder::At:

for (size_t i = 0; i < arraysize(idx_slice_); ++i) {
  if (index <= idx_slice_[i]->max_index()) {
    return idx_slice_[i]->At(index);
  }
}

WDYT?

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:110: size_t index;
nit - move this into the for loop now

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:143: // The object is already in the
constant array, but may have an
Could you pull the check for entry in a larger operand size out into a seperate
if, i.e.:

if (*entry > idx_slice_[operand_size]->slice->max_index()) {
  // The object...
  for ....
}

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:161: if (operand_size ==
idx_slice_[i]->operand_size()) {
Could you make idx_slice_ indexable by operand_size (or have a function which
returns the index for a given operand size? Then you wouldn't need a for loop
here, and I think it would be cleaner.

[oth, 2016-02-25 15:31:23]

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

[commit-bot: I haz the power, 2016-02-25 15:31:37]

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

[oth, 2016-02-25 15:40:15]

Thanks, comments incorporated.

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

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:74:
ConstantArrayBuilder::ConstantArraySlice* ConstantArrayBuilder::IndexToSlice(
On 2016/02/25 13:26:06, rmcilroy wrote:
> This seems a bit overkill to me. I'd prefer just to do the following in
> ConstantArrayBuilder::At:
> 
> for (size_t i = 0; i < arraysize(idx_slice_); ++i) {
>   if (index <= idx_slice_[i]->max_index()) {
>     return idx_slice_[i]->At(index);
>   }
> }
> 
> WDYT?

Compensated in ToFixedArray to avoid 2-3x penalty for wider constant pools.

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:110: size_t index;
On 2016/02/25 13:26:06, rmcilroy wrote:
> nit - move this into the for loop now

Done.

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:143: // The object is already in the
constant array, but may have an
On 2016/02/25 13:26:06, rmcilroy wrote:
> Could you pull the check for entry in a larger operand size out into a
seperate
> if, i.e.:
> 
> if (*entry > idx_slice_[operand_size]->slice->max_index()) {
>   // The object...
>   for ....
> }

Done.

https://codereview.chromium.org/1731893003/diff/40001/src/interpreter/constan...
src/interpreter/constant-array-builder.cc:161: if (operand_size ==
idx_slice_[i]->operand_size()) {
On 2016/02/25 13:26:06, rmcilroy wrote:
> Could you make idx_slice_ indexable by operand_size (or have a function which
> returns the index for a given operand size? Then you wouldn't need a for loop
> here, and I think it would be cleaner.

There is existing code already assumes the enum values of match the actual size.

[commit-bot: I haz the power, 2016-02-25 16:09:53]

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

[commit-bot: I haz the power, 2016-02-25 16:09:53]

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

[oth, 2016-02-25 16:49:12]

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

[oth, 2016-02-25 16:49:13]

+Michi, PTAL utils.h for 32-bit unaligned read.

Thanks

[Michael Starzinger, 2016-02-25 17:23:52]

LGTM on utils.h, didn't look at the rest.

https://codereview.chromium.org/1731893003/diff/100001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/1731893003/diff/100001/src/utils.h#newcode1763
src/utils.h:1763: static inline uint32_t ReadUnalignedUInt32(const void* p) {
nit: Can we move it down by 4 lines? The other function seem to be grouped in
pairs as well.

[oth, 2016-02-26 09:03:10]

Thanks.

https://codereview.chromium.org/1731893003/diff/100001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/1731893003/diff/100001/src/utils.h#newcode1763
src/utils.h:1763: static inline uint32_t ReadUnalignedUInt32(const void* p) {
On 2016/02/25 17:23:52, Michael Starzinger wrote:
> nit: Can we move it down by 4 lines? The other function seem to be grouped in
> pairs as well.

Done.

[rmcilroy, 2016-02-26 09:28:46]

Lgtm, thanks!

[oth, 2016-02-26 09:52:33]

The CQ bit was checked by oth@chromium.org

[oth, 2016-02-26 09:52:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1731893003/#ps120001
(title: "Move ReadUnalignedUInt32.")

[commit-bot: I haz the power, 2016-02-26 09:52:36]

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

[commit-bot: I haz the power, 2016-02-26 10:14:59]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-26 10:16:41]

Description was changed from

==========
[interpreter] Preparation for 32-bit operands.

Extends the constant pool to deal with more slices.

Adds ReadUnalignedUInt32().

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

to

==========
[interpreter] Preparation for 32-bit operands.

Extends the constant pool to deal with more slices.

Adds ReadUnalignedUInt32().

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

Committed: https://crrev.com/1db484f7d8b27405af489b23252b8187c51608dc
Cr-Commit-Position: refs/heads/master@{#34319}
==========

[commit-bot: I haz the power, 2016-02-26 10:16:42]

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