Embedded constant pools.

Add support for Embedded Constant Pools for PPC and Arm

Embed constant pools within their corresponding Code
objects.

This removes support for out-of-line constant pools in favor
of the new approach -- the main advantage being that it
eliminates the need to allocate and manage separate constant
pool array objects.

Currently supported on PPC and ARM.  Enabled by default on
PPC only.

This yields a 6% improvment in Octane on PPC64.

R=danno@chromium.org, svenpanne@chromium.org, bmeurer@chromium.org, rmcilroy@chromium.org, dstence@us.ibm.com, michael_dawson@ca.ibm.com
BUG=chromium:478811
LOG=Y

Committed: https://crrev.com/a9404029343d65f146e3443f5280c40a97e736af
Cr-Commit-Position: refs/heads/master@{#28770}

[MTBrandyberry, 2015-05-08 03:49:37]

Continuation of https://codereview.chromium.org/1030353003/

[MTBrandyberry, 2015-05-19 13:36:28]

On 2015/05/08 03:49:37, mtbrandyberry wrote:
> Continuation of https://codereview.chromium.org/1030353003/

Reminder that this is still pending a review.

[rmcilroy, 2015-05-19 15:01:29]

Apologies for the delay - I'm just back from BlinkOn in Australia. I'm going to
have a look at this either later today or early tomorrow.

[rmcilroy, 2015-05-20 14:32:12]

This is looking great Matthew, this is exaclty the direction I was hoping for -
thanks!

I have a load of comments but most are just nits. I've not looked closely in the
PPC port, I'm assuming you know what your doing here ;). In general, I would
suggest the same comments in PPC as I made in Arm though.

Also, could you please add "BUG=chromium:478811" in the description and update
the title to "Add support for Embedded Constant Pools for PPC and Arm" and
mention in the description that you are removing support for OOL constant pools.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:452: constant_pool_builder_(12, 10),
make the '12' and '10'  constants defined in constants-arm.h

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1083: // An extended constant pool load.
update comment.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:3592: CheckBuffer();
Please add the num_pending_xx_bit_constants_ == 0 DCHECKs here too.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:3894: void Assembler::SetConstantPoolOffset(int pos,
int offset,
I think this would be clearer as something like
PatchConstantPoolAccessInstruction.
Also, rename pos -> pc_offset

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1405: // emitted before any use of db and dd to ensure
that constant pools
nit - update comment to include dq, dp and change "constant pool should be
emitted" to "CheckConstPool() should be called"

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1492: int EmitConstantPool() { return
constant_pool_builder_.Emit(this); }
EmitEmbeddedConstantPool (and add a DCHECK that the flag is true)

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1494: bool ConstantPoolOverflow() const {
nit - how about ConstantPoolAccessIsInOverflow or something like that?

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:362: masm()->EmitConstantPool();
Should we be emitting this here if we are also emitting it in
Assembler::GetCode?

https://codereview.chromium.org/1131783003/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:387: masm()->EmitConstantPool();
Same question as full-codegen

https://codereview.chromium.org/1131783003/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:984: void
MacroAssembler::LoadTargetConstantPoolPointerRegister(Register target) {
/s/LoadTargetConstantPoolPointerRegister/LoadConstantPoolPointerRegisterFromCodeTargetAddress

/s/target/code_target_address

https://codereview.chromium.org/1131783003/diff/1/src/arm64/deoptimizer-arm64.cc
File src/arm64/deoptimizer-arm64.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm64/deoptimizer-arm64...
src/arm64/deoptimizer-arm64.cc:357: // No constant pool support.
No embedded constant .... (Arm64 uses inline constant pools)

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1698
src/assembler.cc:1698: (merged ? ConstantPoolEntry::REGULAR : NextAccess(type));
Please add a DCHECK that merged entries are only in the regular section here and
some kind of unittest would be nice too.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1702
src/assembler.cc:1702: if (entry.sharing_ok() && !merged && access ==
ConstantPoolEntry::REGULAR) {
Did you do any profiling to show that this was more worthwhile than doing
sharing across both REGULAR and OVERFLOW.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1751
src/assembler.cc:1751: }
Could you split the shared entry emitting out to a separate function, it's a bit
confusing that it is lumped in here along with the begin/end setting below.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1769
src/assembler.cc:1769: if (it->merged_index_ < 0) {
nit - would probably be clearer to have a ConstantPoolEntry::is_merged helper
function.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1798
src/assembler.cc:1798: info_[ConstantPoolEntry::DOUBLE].shared_entries.empty());
nit - add ConstantPoolBuilder::is_empty() helper function

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode86
src/assembler.h:86: // Constant pool not supported on this architecture.
Embedded constant... (and below)

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1161
src/assembler.h:1161: merged_index_(sharing_ok ? -1 : -2),
Define and use constants for '-1' and '-2' to clarify their meaning

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1174
src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type == INTPTR) ?
kPointerSize
I think all you need here is:
  return (type == INTPTR) ? kPointerSize : kDoubleSize

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1192
src/assembler.h:1192: class ConstantPoolBuilder BASE_EMBEDDED {
Thanks for factoring this out into common code.  Could you please add some
unittests which test this class and the various corner cases (e.g., overflowing
constant pools, checking the emitted layout etc.).

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1194
src/assembler.h:1194: ConstantPoolBuilder(int ptr_reach, int double_reach);
ptr_reach_bits and double_reach_bits

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1206
src/assembler.h:1206: inline Label* Position() { return &label_; }
/s/Position/EmittedPosition.  Also some comments for each of the public methods
would be nice.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1214
src/assembler.h:1214: struct TypeInfo {
nit - TypeInfo seems a bit confusingly named. How about PerTypeEntryDetails or
similar?

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1227
src/assembler.h:1227: Label label_;
/s/label_/emitted_label_ (and a comment describing what this is would be nice).

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1229
src/assembler.h:1229: };
two newlines here

https://codereview.chromium.org/1131783003/diff/1/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:132: AssembleConstantPool();
Do we need this here too if we are doing it in GetCode? It seems like there are
quite a few places we are emitting the constant pools when ideally I would hope
there only needed to be a single place in GetCode().

https://codereview.chromium.org/1131783003/diff/1/src/frames.cc
File src/frames.cc (left):

https://codereview.chromium.org/1131783003/diff/1/src/frames.cc#oldcode326
src/frames.cc:326: }
Why are you removing this?

https://codereview.chromium.org/1131783003/diff/1/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode128
src/frames.h:128: static const int kConstantPoolOffset = kCPSlotSize ? -1 *
kPointerSize : 0;
nit - align '='

https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode133
src/frames.h:133: static const int kLastObjectOffset = kContextOffset;
ditto

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:4627: slot_type = SlotsBuffer::OBJECT_SLOT;
why are these changes to add OBJECT_SLOT to the mark compact collector required?
Can they be made in a different CL?

https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h#n...
src/ia32/assembler-ia32.h:1452: void dp(uintptr_t data) { dd(data); }
I don't think you need dq and dp for the architectures which don't support the
constant pool. Could you remove all of these to keep the CL smaller and we can
add them back in the future if they become necessary.

https://codereview.chromium.org/1131783003/diff/1/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/ia32/deoptimizer-ia32.c...
src/ia32/deoptimizer-ia32.cc:426: // No constant pool support.
embedded

https://codereview.chromium.org/1131783003/diff/1/src/mips/deoptimizer-mips.cc
File src/mips/deoptimizer-mips.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/mips/deoptimizer-mips.c...
src/mips/deoptimizer-mips.cc:402: // No constant pool support.
embedded

https://codereview.chromium.org/1131783003/diff/1/src/mips64/deoptimizer-mips...
File src/mips64/deoptimizer-mips64.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/mips64/deoptimizer-mips...
src/mips64/deoptimizer-mips64.cc:406: // No constant pool support.
embedded

https://codereview.chromium.org/1131783003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/objects.cc#newcode11843
src/objects.cc:11843: int constant_offset =
nit - constant_pool_offset

https://codereview.chromium.org/1131783003/diff/1/src/objects.cc#newcode11847
src/objects.cc:11847: int code_size = Min(safepoint_offset, back_edge_offset);
Do we want the Min of constant_pool_offset as well as safepoint_offset and
back_edge_offset?

https://codereview.chromium.org/1131783003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/objects.h#newcode5300
src/objects.h:5300: static const int kCPSize =
FLAG_enable_embedded_constant_pool ? kIntSize : 0;
nit - kConstantPoolSize

https://codereview.chromium.org/1131783003/diff/1/src/ppc/assembler-ppc.cc
File src/ppc/assembler-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/ppc/assembler-ppc.cc#ne...
src/ppc/assembler-ppc.cc:208: constant_pool_builder_(15, 15),  // 15-bit
unsigned reach
same comments as Arm

https://codereview.chromium.org/1131783003/diff/1/src/x87/deoptimizer-x87.cc
File src/x87/deoptimizer-x87.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/x87/deoptimizer-x87.cc#...
src/x87/deoptimizer-x87.cc:465: // No constant pool support.
embedded

[MTBrandyberry, 2015-05-20 16:20:07]

On 2015/05/20 14:32:12, rmcilroy wrote:
> This is looking great Matthew, this is exaclty the direction I was hoping for
-
> thanks!
> 
> I have a load of comments but most are just nits. I've not looked closely in
the
> PPC port, I'm assuming you know what your doing here ;). In general, I would
> suggest the same comments in PPC as I made in Arm though.
> 
> Also, could you please add "BUG=chromium:478811" in the description and update
> the title to "Add support for Embedded Constant Pools for PPC and Arm" and
> mention in the description that you are removing support for OOL constant
pools.
> 

Thanks. Comments look reasonable.  I'll get started on the next iteration.

[MTBrandyberry, 2015-05-20 22:28:23]

My responses below.  Highlights are that I agree with most of the comments and
have provided clarifications for a small number.  I'll work on providing the
unittest now (but wanted to address the others and get the re-review started in
parallel).

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:452: constant_pool_builder_(12, 10),
On 2015/05/20 14:32:10, rmcilroy wrote:
> make the '12' and '10'  constants defined in constants-arm.h

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1083: // An extended constant pool load.
On 2015/05/20 14:32:10, rmcilroy wrote:
> update comment.

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:3592: CheckBuffer();
On 2015/05/20 14:32:10, rmcilroy wrote:
> Please add the num_pending_xx_bit_constants_ == 0 DCHECKs here too.

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:3894: void Assembler::SetConstantPoolOffset(int pos,
int offset,
On 2015/05/20 14:32:10, rmcilroy wrote:
> I think this would be clearer as something like
> PatchConstantPoolAccessInstruction.
> Also, rename pos -> pc_offset

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1405: // emitted before any use of db and dd to ensure
that constant pools
On 2015/05/20 14:32:10, rmcilroy wrote:
> nit - update comment to include dq, dp and change "constant pool should be
> emitted" to "CheckConstPool() should be called"

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1492: int EmitConstantPool() { return
constant_pool_builder_.Emit(this); }
On 2015/05/20 14:32:10, rmcilroy wrote:
> EmitEmbeddedConstantPool (and add a DCHECK that the flag is true)

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1494: bool ConstantPoolOverflow() const {
On 2015/05/20 14:32:10, rmcilroy wrote:
> nit - how about ConstantPoolAccessIsInOverflow or something like that?

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:362: masm()->EmitConstantPool();
On 2015/05/20 14:32:10, rmcilroy wrote:
> Should we be emitting this here if we are also emitting it in
> Assembler::GetCode?

Currently, the embedded constant pool is emitted immediately following the
instructions but prior to any other tables (e.g. safepoint, back-edge).  The
intent was to keep the constant pool related logic together and have the
constant pool area immediately follow code in the disassembly.  I'll concede
that there is no strict reason for this ordering.  (Note, however, that the
constant pool builder is smart enough to emit the table only on the first
invocation).

Let me know if this is a significant issue and I will modify it to emit at the
end.

https://codereview.chromium.org/1131783003/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:387: masm()->EmitConstantPool();
On 2015/05/20 14:32:10, rmcilroy wrote:
> Same question as full-codegen

See earlier response.

https://codereview.chromium.org/1131783003/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:984: void
MacroAssembler::LoadTargetConstantPoolPointerRegister(Register target) {
On 2015/05/20 14:32:10, rmcilroy wrote:
>
/s/LoadTargetConstantPoolPointerRegister/LoadConstantPoolPointerRegisterFromCodeTargetAddress
> 
> /s/target/code_target_address

Done.

https://codereview.chromium.org/1131783003/diff/1/src/arm64/deoptimizer-arm64.cc
File src/arm64/deoptimizer-arm64.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm64/deoptimizer-arm64...
src/arm64/deoptimizer-arm64.cc:357: // No constant pool support.
On 2015/05/20 14:32:10, rmcilroy wrote:
> No embedded constant .... (Arm64 uses inline constant pools)

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1698
src/assembler.cc:1698: (merged ? ConstantPoolEntry::REGULAR : NextAccess(type));
On 2015/05/20 14:32:11, rmcilroy wrote:
> Please add a DCHECK that merged entries are only in the regular section here
and
> some kind of unittest would be nice too.

Done.  Note that the check asserting that the offset is within reach (during
emit, prior to patching the instructions) provides a more comprehensive sanity
check for this.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1702
src/assembler.cc:1702: if (entry.sharing_ok() && !merged && access ==
ConstantPoolEntry::REGULAR) {
On 2015/05/20 14:32:10, rmcilroy wrote:
> Did you do any profiling to show that this was more worthwhile than doing
> sharing across both REGULAR and OVERFLOW.

If left unbounded, the old O(n^2) search caused unacceptable compile times that
were actually noticeable to the user.  I found this out the hard way via octane.
mandreel's global_init function has so many constants eligible for sharing that
it basically hung the benchmark.

Using the the regular area as the limit provides opportunities for
simplification vs using another arbitrary limit.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1751
src/assembler.cc:1751: }
On 2015/05/20 14:32:11, rmcilroy wrote:
> Could you split the shared entry emitting out to a separate function, it's a
bit
> confusing that it is lumped in here along with the begin/end setting below.

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1769
src/assembler.cc:1769: if (it->merged_index_ < 0) {
On 2015/05/20 14:32:11, rmcilroy wrote:
> nit - would probably be clearer to have a ConstantPoolEntry::is_merged helper
> function.

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1798
src/assembler.cc:1798: info_[ConstantPoolEntry::DOUBLE].shared_entries.empty());
On 2015/05/20 14:32:11, rmcilroy wrote:
> nit - add ConstantPoolBuilder::is_empty() helper function

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode86
src/assembler.h:86: // Constant pool not supported on this architecture.
On 2015/05/20 14:32:11, rmcilroy wrote:
> Embedded constant... (and below)

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1161
src/assembler.h:1161: merged_index_(sharing_ok ? -1 : -2),
On 2015/05/20 14:32:11, rmcilroy wrote:
> Define and use constants for '-1' and '-2' to clarify their meaning

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1174
src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type == INTPTR) ?
kPointerSize
On 2015/05/20 14:32:11, rmcilroy wrote:
> I think all you need here is:
>   return (type == INTPTR) ? kPointerSize : kDoubleSize

My thinking was that the compiler could eliminate the condition all together on
64-bit architectures.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1192
src/assembler.h:1192: class ConstantPoolBuilder BASE_EMBEDDED {
On 2015/05/20 14:32:11, rmcilroy wrote:
> Thanks for factoring this out into common code.  Could you please add some
> unittests which test this class and the various corner cases (e.g.,
overflowing
> constant pools, checking the emitted layout etc.).

I will resolve the other comments here in the next iteration and tackle the
unittests in parallel with the 2nd review.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1194
src/assembler.h:1194: ConstantPoolBuilder(int ptr_reach, int double_reach);
On 2015/05/20 14:32:11, rmcilroy wrote:
> ptr_reach_bits and double_reach_bits

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1206
src/assembler.h:1206: inline Label* Position() { return &label_; }
On 2015/05/20 14:32:11, rmcilroy wrote:
> /s/Position/EmittedPosition.  Also some comments for each of the public
methods
> would be nice.

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1214
src/assembler.h:1214: struct TypeInfo {
On 2015/05/20 14:32:11, rmcilroy wrote:
> nit - TypeInfo seems a bit confusingly named. How about PerTypeEntryDetails or
> similar?

Done.  PerTypeEntryInfo?

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1227
src/assembler.h:1227: Label label_;
On 2015/05/20 14:32:11, rmcilroy wrote:
> /s/label_/emitted_label_ (and a comment describing what this is would be
nice).

Done.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1229
src/assembler.h:1229: };
On 2015/05/20 14:32:11, rmcilroy wrote:
> two newlines here

Done.

https://codereview.chromium.org/1131783003/diff/1/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:132: AssembleConstantPool();
On 2015/05/20 14:32:11, rmcilroy wrote:
> Do we need this here too if we are doing it in GetCode? It seems like there
are
> quite a few places we are emitting the constant pools when ideally I would
hope
> there only needed to be a single place in GetCode().

See earlier response.

https://codereview.chromium.org/1131783003/diff/1/src/frames.cc
File src/frames.cc (left):

https://codereview.chromium.org/1131783003/diff/1/src/frames.cc#oldcode326
src/frames.cc:326: }
On 2015/05/20 14:32:11, rmcilroy wrote:
> Why are you removing this?

By definition, the an exit frame's pc_address is above sp (see
ExitFrame::FillState).  Thus, this check always fails.

This appears to be harmless for frames without the constant pool slot, but
causes real issues otherwise.  IIRC, it leads to clobbering constant pool
register upon return in some cases.

https://codereview.chromium.org/1131783003/diff/1/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode128
src/frames.h:128: static const int kConstantPoolOffset = kCPSlotSize ? -1 *
kPointerSize : 0;
On 2015/05/20 14:32:11, rmcilroy wrote:
> nit - align '='

git cl format did this.  Should I override?

https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode133
src/frames.h:133: static const int kLastObjectOffset = kContextOffset;
On 2015/05/20 14:32:11, rmcilroy wrote:
> ditto

see above

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:4627: slot_type = SlotsBuffer::OBJECT_SLOT;
On 2015/05/20 14:32:11, rmcilroy wrote:
> why are these changes to add OBJECT_SLOT to the mark compact collector
required?
> Can they be made in a different CL?

This is required. 
https://chromium.googlesource.com/v8/v8/+/7ad9980d995b0381046ee8677861491ef6d...
effectively broke ool/embedded constant pools.  I believe this restores
functionality without violating the intent of that change.

https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h#n...
src/ia32/assembler-ia32.h:1452: void dp(uintptr_t data) { dd(data); }
On 2015/05/20 14:32:11, rmcilroy wrote:
> I don't think you need dq and dp for the architectures which don't support the
> constant pool. Could you remove all of these to keep the CL smaller and we can
> add them back in the future if they become necessary.

The common constant pool builder code references these symbols.  I could ifdef
that code, but prior experience has taught me that is frowned upon. :)

https://codereview.chromium.org/1131783003/diff/1/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/ia32/deoptimizer-ia32.c...
src/ia32/deoptimizer-ia32.cc:426: // No constant pool support.
On 2015/05/20 14:32:11, rmcilroy wrote:
> embedded

Done.

https://codereview.chromium.org/1131783003/diff/1/src/mips/deoptimizer-mips.cc
File src/mips/deoptimizer-mips.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/mips/deoptimizer-mips.c...
src/mips/deoptimizer-mips.cc:402: // No constant pool support.
On 2015/05/20 14:32:11, rmcilroy wrote:
> embedded

Done.

https://codereview.chromium.org/1131783003/diff/1/src/mips64/deoptimizer-mips...
File src/mips64/deoptimizer-mips64.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/mips64/deoptimizer-mips...
src/mips64/deoptimizer-mips64.cc:406: // No constant pool support.
On 2015/05/20 14:32:11, rmcilroy wrote:
> embedded

Done.

https://codereview.chromium.org/1131783003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/objects.cc#newcode11843
src/objects.cc:11843: int constant_offset =
On 2015/05/20 14:32:12, rmcilroy wrote:
> nit - constant_pool_offset

Done.

https://codereview.chromium.org/1131783003/diff/1/src/objects.cc#newcode11847
src/objects.cc:11847: int code_size = Min(safepoint_offset, back_edge_offset);
On 2015/05/20 14:32:12, rmcilroy wrote:
> Do we want the Min of constant_pool_offset as well as safepoint_offset and
> back_edge_offset?

Effectively yes -- that's result of the 'end' calculation below. However, I need
to preserve this value of 'code_size' for comparison against
constant_pool_offset below.  Again, this code assumes that the constant pool is
emitted immediately following the instructions.

https://codereview.chromium.org/1131783003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/objects.h#newcode5300
src/objects.h:5300: static const int kCPSize =
FLAG_enable_embedded_constant_pool ? kIntSize : 0;
On 2015/05/20 14:32:12, rmcilroy wrote:
> nit - kConstantPoolSize

Done.

https://codereview.chromium.org/1131783003/diff/1/src/ppc/assembler-ppc.cc
File src/ppc/assembler-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/ppc/assembler-ppc.cc#ne...
src/ppc/assembler-ppc.cc:208: constant_pool_builder_(15, 15),  // 15-bit
unsigned reach
On 2015/05/20 14:32:12, rmcilroy wrote:
> same comments as Arm

Done.

https://codereview.chromium.org/1131783003/diff/1/src/x87/deoptimizer-x87.cc
File src/x87/deoptimizer-x87.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/x87/deoptimizer-x87.cc#...
src/x87/deoptimizer-x87.cc:465: // No constant pool support.
On 2015/05/20 14:32:12, rmcilroy wrote:
> embedded

Done.

[rmcilroy, 2015-05-22 12:21:22]

I'll do a last pass once you have the tests.

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:362: masm()->EmitConstantPool();
On 2015/05/20 22:28:21, mtbrandyberry wrote:
> On 2015/05/20 14:32:10, rmcilroy wrote:
> > Should we be emitting this here if we are also emitting it in
> > Assembler::GetCode?
> 
> Currently, the embedded constant pool is emitted immediately following the
> instructions but prior to any other tables (e.g. safepoint, back-edge).  The
> intent was to keep the constant pool related logic together and have the
> constant pool area immediately follow code in the disassembly.  I'll concede
> that there is no strict reason for this ordering.  (Note, however, that the
> constant pool builder is smart enough to emit the table only on the first
> invocation).
> 
> Let me know if this is a significant issue and I will modify it to emit at the
> end.

I think this would be better right at the end just before the relocinfo (which
is what describes the constant pool anyway). I don't think this should make the
disassembly any more complex, but if it does let me know and I'll re-evaluate.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1698
src/assembler.cc:1698: (merged ? ConstantPoolEntry::REGULAR : NextAccess(type));
On 2015/05/20 22:28:22, mtbrandyberry wrote:
> On 2015/05/20 14:32:11, rmcilroy wrote:
> > Please add a DCHECK that merged entries are only in the regular section here
> and
> > some kind of unittest would be nice too.
> 
> Done.  Note that the check asserting that the offset is within reach (during
> emit, prior to patching the instructions) provides a more comprehensive sanity
> check for this.

Yes, but it is also good to get DCHECKs early where possible to make debuggging
easier. Thanks for adding the DCHECK.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1702
src/assembler.cc:1702: if (entry.sharing_ok() && !merged && access ==
ConstantPoolEntry::REGULAR) {
On 2015/05/20 22:28:22, mtbrandyberry wrote:
> On 2015/05/20 14:32:10, rmcilroy wrote:
> > Did you do any profiling to show that this was more worthwhile than doing
> > sharing across both REGULAR and OVERFLOW.
> 
> If left unbounded, the old O(n^2) search caused unacceptable compile times
that
> were actually noticeable to the user.  I found this out the hard way via
octane.
> mandreel's global_init function has so many constants eligible for sharing
that
> it basically hung the benchmark.
> 
> Using the the regular area as the limit provides opportunities for
> simplification vs using another arbitrary limit.

Right, thanks for the explination. I had issues with mandreel's global_init
function as well.  In this case I'm fine with this.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1174
src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type == INTPTR) ?
kPointerSize
On 2015/05/20 22:28:22, mtbrandyberry wrote:
> On 2015/05/20 14:32:11, rmcilroy wrote:
> > I think all you need here is:
> >   return (type == INTPTR) ? kPointerSize : kDoubleSize
> 
> My thinking was that the compiler could eliminate the condition all together
on
> 64-bit architectures.

If the compiler is smart enough to do that, then it is also probably smart
enough to do constant propigation and work out kPointerSize and kDoubleSize are
the same and also eliminate the condition. In anycase, I don't think this
microoptimisation is worth the reduced readability unless you have specific
numbers to back up it being worthwhile.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1214
src/assembler.h:1214: struct TypeInfo {
On 2015/05/20 22:28:22, mtbrandyberry wrote:
> On 2015/05/20 14:32:11, rmcilroy wrote:
> > nit - TypeInfo seems a bit confusingly named. How about PerTypeEntryDetails
or
> > similar?
> 
> Done.  PerTypeEntryInfo?

SGTM

https://codereview.chromium.org/1131783003/diff/1/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode128
src/frames.h:128: static const int kConstantPoolOffset = kCPSlotSize ? -1 *
kPointerSize : 0;
On 2015/05/20 22:28:22, mtbrandyberry wrote:
> On 2015/05/20 14:32:11, rmcilroy wrote:
> > nit - align '='
> 
> git cl format did this.  Should I override?

Ahh right, in that case could you touch all the constants in this block so git
cl format lines makes them all two-space instead of lined up.

https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h#n...
src/ia32/assembler-ia32.h:1452: void dp(uintptr_t data) { dd(data); }
On 2015/05/20 22:28:22, mtbrandyberry wrote:
> On 2015/05/20 14:32:11, rmcilroy wrote:
> > I don't think you need dq and dp for the architectures which don't support
the
> > constant pool. Could you remove all of these to keep the CL smaller and we
can
> > add them back in the future if they become necessary.
> 
> The common constant pool builder code references these symbols.  I could ifdef
> that code, but prior experience has taught me that is frowned upon. :)

Right enough. In that case this is fine.

[MTBrandyberry, 2015-05-26 14:57:46]

On 2015/05/22 12:21:22, rmcilroy wrote:
> I'll do a last pass once you have the tests.
> 
> https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc
> File src/arm/full-codegen-arm.cc (right):
> 
>
https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc...
> src/arm/full-codegen-arm.cc:362: masm()->EmitConstantPool();
> On 2015/05/20 22:28:21, mtbrandyberry wrote:
> > On 2015/05/20 14:32:10, rmcilroy wrote:
> > > Should we be emitting this here if we are also emitting it in
> > > Assembler::GetCode?
> > 
> > Currently, the embedded constant pool is emitted immediately following the
> > instructions but prior to any other tables (e.g. safepoint, back-edge).  The
> > intent was to keep the constant pool related logic together and have the
> > constant pool area immediately follow code in the disassembly.  I'll concede
> > that there is no strict reason for this ordering.  (Note, however, that the
> > constant pool builder is smart enough to emit the table only on the first
> > invocation).
> > 
> > Let me know if this is a significant issue and I will modify it to emit at
the
> > end.
> 
> I think this would be better right at the end just before the relocinfo (which
> is what describes the constant pool anyway). I don't think this should make
the
> disassembly any more complex, but if it does let me know and I'll re-evaluate.
> 
> https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc
> File src/assembler.cc (right):
> 
> https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1698
> src/assembler.cc:1698: (merged ? ConstantPoolEntry::REGULAR :
NextAccess(type));
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > Please add a DCHECK that merged entries are only in the regular section
here
> > and
> > > some kind of unittest would be nice too.
> > 
> > Done.  Note that the check asserting that the offset is within reach (during
> > emit, prior to patching the instructions) provides a more comprehensive
sanity
> > check for this.
> 
> Yes, but it is also good to get DCHECKs early where possible to make
debuggging
> easier. Thanks for adding the DCHECK.
> 
> https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1702
> src/assembler.cc:1702: if (entry.sharing_ok() && !merged && access ==
> ConstantPoolEntry::REGULAR) {
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:10, rmcilroy wrote:
> > > Did you do any profiling to show that this was more worthwhile than doing
> > > sharing across both REGULAR and OVERFLOW.
> > 
> > If left unbounded, the old O(n^2) search caused unacceptable compile times
> that
> > were actually noticeable to the user.  I found this out the hard way via
> octane.
> > mandreel's global_init function has so many constants eligible for sharing
> that
> > it basically hung the benchmark.
> > 
> > Using the the regular area as the limit provides opportunities for
> > simplification vs using another arbitrary limit.
> 
> Right, thanks for the explination. I had issues with mandreel's global_init
> function as well.  In this case I'm fine with this.
> 
> https://codereview.chromium.org/1131783003/diff/1/src/assembler.h
> File src/assembler.h (right):
> 
> https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1174
> src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type == INTPTR)
?
> kPointerSize
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > I think all you need here is:
> > >   return (type == INTPTR) ? kPointerSize : kDoubleSize
> > 
> > My thinking was that the compiler could eliminate the condition all together
> on
> > 64-bit architectures.
> 
> If the compiler is smart enough to do that, then it is also probably smart
> enough to do constant propigation and work out kPointerSize and kDoubleSize
are
> the same and also eliminate the condition. In anycase, I don't think this
> microoptimisation is worth the reduced readability unless you have specific
> numbers to back up it being worthwhile.
> 
> https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1214
> src/assembler.h:1214: struct TypeInfo {
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > nit - TypeInfo seems a bit confusingly named. How about
PerTypeEntryDetails
> or
> > > similar?
> > 
> > Done.  PerTypeEntryInfo?
> 
> SGTM
> 
> https://codereview.chromium.org/1131783003/diff/1/src/frames.h
> File src/frames.h (right):
> 
> https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode128
> src/frames.h:128: static const int kConstantPoolOffset = kCPSlotSize ? -1 *
> kPointerSize : 0;
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > nit - align '='
> > 
> > git cl format did this.  Should I override?
> 
> Ahh right, in that case could you touch all the constants in this block so git
> cl format lines makes them all two-space instead of lined up.
> 
> https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h
> File src/ia32/assembler-ia32.h (right):
> 
>
https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h#n...
> src/ia32/assembler-ia32.h:1452: void dp(uintptr_t data) { dd(data); }
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > I don't think you need dq and dp for the architectures which don't support
> the
> > > constant pool. Could you remove all of these to keep the CL smaller and we
> can
> > > add them back in the future if they become necessary.
> > 
> > The common constant pool builder code references these symbols.  I could
ifdef
> > that code, but prior experience has taught me that is frowned upon. :)
> 
> Right enough. In that case this is fine.

I was out of the office for a few days.  Picking this back up.

[MTBrandyberry, 2015-05-26 19:19:05]

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:362: masm()->EmitConstantPool();
On 2015/05/22 12:21:21, rmcilroy wrote:
> On 2015/05/20 22:28:21, mtbrandyberry wrote:
> > On 2015/05/20 14:32:10, rmcilroy wrote:
> > > Should we be emitting this here if we are also emitting it in
> > > Assembler::GetCode?
> > 
> > Currently, the embedded constant pool is emitted immediately following the
> > instructions but prior to any other tables (e.g. safepoint, back-edge).  The
> > intent was to keep the constant pool related logic together and have the
> > constant pool area immediately follow code in the disassembly.  I'll concede
> > that there is no strict reason for this ordering.  (Note, however, that the
> > constant pool builder is smart enough to emit the table only on the first
> > invocation).
> > 
> > Let me know if this is a significant issue and I will modify it to emit at
the
> > end.
> 
> I think this would be better right at the end just before the relocinfo (which
> is what describes the constant pool anyway). I don't think this should make
the
> disassembly any more complex, but if it does let me know and I'll re-evaluate.

Acknowledged.

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1174
src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type == INTPTR) ?
kPointerSize
On 2015/05/22 12:21:22, rmcilroy wrote:
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > I think all you need here is:
> > >   return (type == INTPTR) ? kPointerSize : kDoubleSize
> > 
> > My thinking was that the compiler could eliminate the condition all together
> on
> > 64-bit architectures.
> 
> If the compiler is smart enough to do that, then it is also probably smart
> enough to do constant propigation and work out kPointerSize and kDoubleSize
are
> the same and also eliminate the condition. In anycase, I don't think this
> microoptimisation is worth the reduced readability unless you have specific
> numbers to back up it being worthwhile.

Acknowledged.

https://codereview.chromium.org/1131783003/diff/1/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode128
src/frames.h:128: static const int kConstantPoolOffset = kCPSlotSize ? -1 *
kPointerSize : 0;
On 2015/05/22 12:21:22, rmcilroy wrote:
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > nit - align '='
> > 
> > git cl format did this.  Should I override?
> 
> Ahh right, in that case could you touch all the constants in this block so git
> cl format lines makes them all two-space instead of lined up.

Acknowledged.

[MTBrandyberry, 2015-05-27 22:08:42]

Patch Set 3 addresses remaining issues.

[rmcilroy, 2015-06-01 09:52:07]

rmcilroy@chromium.org changed reviewers:
+ ishell@chromium.org
- bmeurer@chromium.org, danno@chromium.org, dstence@us.ibm.com,
michael_dawson@ca.ibm.com, svenpanne@chromium.org

[rmcilroy, 2015-06-01 09:52:09]

+ishell

Igor: Could you have a quick look at the changes in mark-compact.cc and check
they don't break slots filtering?

Other than that looks good, with one last question in lithium-codegen-ppc.cc and
a couple of small nits.

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:4627: slot_type = SlotsBuffer::OBJECT_SLOT;
On 2015/05/20 22:28:22, mtbrandyberry wrote:
> On 2015/05/20 14:32:11, rmcilroy wrote:
> > why are these changes to add OBJECT_SLOT to the mark compact collector
> required?
> > Can they be made in a different CL?
> 
> This is required. 
>
https://chromium.googlesource.com/v8/v8/+/7ad9980d995b0381046ee8677861491ef6d...
> effectively broke ool/embedded constant pools.  I believe this restores
> functionality without violating the intent of that change.

ishell@: Could you have a quick look at this change an check it won't cause
issues with slot filtering again.

https://codereview.chromium.org/1131783003/diff/40001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:478: offset = EmitEmbeddedConstantPool();
/s/offset/constant_pool_offset

https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h#newcode...
src/assembler.h:1154: // Embedded constant pool support
ConstntPoolEntry is used by both embedded and inline constant pools now right? 
Change the comment here to "Constant pool support" and add one above
ConstantPoolBuilder with "Embedded constant pool support"

https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h#newcode...
src/assembler.h:1189: friend class ConstantPoolBuilder;
is this only for setting merged_index_? If so, I would prefer to add a
set_merged_index() function rather than requiring that the classes be friended

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
File src/ppc/lithium-codegen-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure in
~Label()
Won't this cause issues in non-debug code if the label is getting deleted before
the constant pool is emitted?

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
File test/cctest/test-constantpool.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:1: // Copyright 2013 the V8 project authors.
All rights reserved.
2015. Also you just need the short copyright notice now:

// Copyright 2015 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

https://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:43: const int kReach = 1 << kReachBits;
nit - could you add one test which has dblReach different from intReach to check
that works as intended.

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:218: }
It would be nice if you could test the emitting logic as well, though I realize
this would be tricky to do in a non-arch specific way. Would it be possible to
add some test of the emitting logic to test-assembler-ppc?

[Igor Sheludko, 2015-06-01 10:42:30]

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:4627: slot_type = SlotsBuffer::OBJECT_SLOT;
On 2015/06/01 09:52:08, rmcilroy wrote:
> On 2015/05/20 22:28:22, mtbrandyberry wrote:
> > On 2015/05/20 14:32:11, rmcilroy wrote:
> > > why are these changes to add OBJECT_SLOT to the mark compact collector
> > required?
> > > Can they be made in a different CL?
> > 
> > This is required. 
> >
>
https://chromium.googlesource.com/v8/v8/+/7ad9980d995b0381046ee8677861491ef6d...
> > effectively broke ool/embedded constant pools.  I believe this restores
> > functionality without violating the intent of that change.
> 
> ishell@: Could you have a quick look at this change an check it won't cause
> issues with slot filtering again.

Sgtm. Since we have a typed slot here the slots filtering should not be
affected.

[MTBrandyberry, 2015-06-01 21:01:31]

https://codereview.chromium.org/1131783003/diff/40001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:478: offset = EmitEmbeddedConstantPool();
On 2015/06/01 09:52:08, rmcilroy wrote:
> /s/offset/constant_pool_offset

Done.

https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h#newcode...
src/assembler.h:1154: // Embedded constant pool support
On 2015/06/01 09:52:08, rmcilroy wrote:
> ConstntPoolEntry is used by both embedded and inline constant pools now right?

> Change the comment here to "Constant pool support" and add one above
> ConstantPoolBuilder with "Embedded constant pool support"

Done.

https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h#newcode...
src/assembler.h:1189: friend class ConstantPoolBuilder;
On 2015/06/01 09:52:09, rmcilroy wrote:
> is this only for setting merged_index_? If so, I would prefer to add a
> set_merged_index() function rather than requiring that the classes be friended

Done.

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
File src/ppc/lithium-codegen-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure in
~Label()
On 2015/06/01 09:52:09, rmcilroy wrote:
> Won't this cause issues in non-debug code if the label is getting deleted
before
> the constant pool is emitted?

Not quite.  The label gets deleted upon destruction of the assembler object. 
The issue here is that the embedded constant pool label may be linked to in the
prologue, but never bound since this path aborts the compilation prior to
reaching GetCode (where the constant pool would have been emitted).

If there is a better way to work around  this, let me know.  It seems like the
labels used for deferred code generation could also run into this.  Perhaps the
tests just don't exercise that scenario?

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
File test/cctest/test-constantpool.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:1: // Copyright 2013 the V8 project authors.
All rights reserved.
On 2015/06/01 09:52:09, rmcilroy wrote:
> 2015. Also you just need the short copyright notice now:
> 
> // Copyright 2015 the V8 project authors. All rights reserved.
> // Use of this source code is governed by a BSD-style license that can be
> // found in the LICENSE file.
> 
> https://www.chromium.org/developers/coding-style#TOC-File-headers

Done.

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:43: const int kReach = 1 << kReachBits;
On 2015/06/01 09:52:09, rmcilroy wrote:
> nit - could you add one test which has dblReach different from intReach to
check
> that works as intended.

Done.

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:218: }
On 2015/06/01 09:52:09, rmcilroy wrote:
> It would be nice if you could test the emitting logic as well, though I
realize
> this would be tricky to do in a non-arch specific way. Would it be possible to
> add some test of the emitting logic to test-assembler-ppc?

I considered this, however other than validating the expected size of the
emitted pool, there isn't much else to assert on that adds value beyond just
running the rest of the tests with embedded pools enabled.

I'd like to consider this further as a separate CL. PPC in general needs better
coverage in test-assembler-ppc.

[rmcilroy, 2015-06-02 13:57:19]

rmcilroy@chromium.org changed reviewers:
+ rodolph.perfetta@arm.com

[rmcilroy, 2015-06-02 13:57:21]

lgtm once EmittedLabel is removed from the ConstantPoolBuilder (otherwise let's
do another review round).

+Rodolph as FYI on Arm changes.

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
File src/ppc/lithium-codegen-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure in
~Label()
On 2015/06/01 21:01:30, mtbrandyberry wrote:
> On 2015/06/01 09:52:09, rmcilroy wrote:
> > Won't this cause issues in non-debug code if the label is getting deleted
> before
> > the constant pool is emitted?
> 
> Not quite.  The label gets deleted upon destruction of the assembler object. 
> The issue here is that the embedded constant pool label may be linked to in
the
> prologue, but never bound since this path aborts the compilation prior to
> reaching GetCode (where the constant pool would have been emitted).
> 
> If there is a better way to work around  this, let me know.  It seems like the
> labels used for deferred code generation could also run into this.  Perhaps
the
> tests just don't exercise that scenario?

The more I think about it the less keen I am to be storing this as a label in
the constant pool builder object. Could you just do the same as on Arm instead
(getting the offset from the code object and adding that to the code target
address)?

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
File test/cctest/test-constantpool.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:218: }
On 2015/06/01 21:01:31, mtbrandyberry wrote:
> On 2015/06/01 09:52:09, rmcilroy wrote:
> > It would be nice if you could test the emitting logic as well, though I
> realize
> > this would be tricky to do in a non-arch specific way. Would it be possible
to
> > add some test of the emitting logic to test-assembler-ppc?
> 
> I considered this, however other than validating the expected size of the
> emitted pool, there isn't much else to assert on that adds value beyond just
> running the rest of the tests with embedded pools enabled.
> 
> I'd like to consider this further as a separate CL. PPC in general needs
better
> coverage in test-assembler-ppc.

OK fair enough.

https://codereview.chromium.org/1131783003/diff/60001/test/cctest/test-consta...
File test/cctest/test-constantpool.cc (right):

https://codereview.chromium.org/1131783003/diff/60001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:1: // Copyright 2013 the V8 project authors.
All rights reserved.
Still need to update date to 2015.

[MTBrandyberry, 2015-06-02 15:36:11]

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
File src/ppc/lithium-codegen-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure in
~Label()
On 2015/06/02 13:57:21, rmcilroy wrote:
> On 2015/06/01 21:01:30, mtbrandyberry wrote:
> > On 2015/06/01 09:52:09, rmcilroy wrote:
> > > Won't this cause issues in non-debug code if the label is getting deleted
> > before
> > > the constant pool is emitted?
> > 
> > Not quite.  The label gets deleted upon destruction of the assembler object.

> > The issue here is that the embedded constant pool label may be linked to in
> the
> > prologue, but never bound since this path aborts the compilation prior to
> > reaching GetCode (where the constant pool would have been emitted).
> > 
> > If there is a better way to work around  this, let me know.  It seems like
the
> > labels used for deferred code generation could also run into this.  Perhaps
> the
> > tests just don't exercise that scenario?
> 
> The more I think about it the less keen I am to be storing this as a label in
> the constant pool builder object. Could you just do the same as on Arm instead
> (getting the offset from the code object and adding that to the code target
> address)? 

Loading the offset from the code object is problematic on PPC due to the lack of
pc-relative addressing.

I disagree that this scenario or the builder's use of a label is inherently bad.
 As far as I can tell, it's no different than any other use of a label prior to
binding it.  If anything, the problem is that the Label destructor's DCHECK is
invalid for cases such as this where the compilation was aborted and the code
will be discarded.

https://codereview.chromium.org/1131783003/diff/60001/test/cctest/test-consta...
File test/cctest/test-constantpool.cc (right):

https://codereview.chromium.org/1131783003/diff/60001/test/cctest/test-consta...
test/cctest/test-constantpool.cc:1: // Copyright 2013 the V8 project authors.
All rights reserved.
On 2015/06/02 13:57:21, rmcilroy wrote:
> Still need to update date to 2015.

Not sure how that happened.  Will fix.

[rmcilroy, 2015-06-02 16:40:12]

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
File src/ppc/lithium-codegen-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure in
~Label()
On 2015/06/02 15:36:10, mtbrandyberry wrote:
> On 2015/06/02 13:57:21, rmcilroy wrote:
> > On 2015/06/01 21:01:30, mtbrandyberry wrote:
> > > On 2015/06/01 09:52:09, rmcilroy wrote:
> > > > Won't this cause issues in non-debug code if the label is getting
deleted
> > > before
> > > > the constant pool is emitted?
> > > 
> > > Not quite.  The label gets deleted upon destruction of the assembler
object.
> 
> > > The issue here is that the embedded constant pool label may be linked to
in
> > the
> > > prologue, but never bound since this path aborts the compilation prior to
> > > reaching GetCode (where the constant pool would have been emitted).
> > > 
> > > If there is a better way to work around  this, let me know.  It seems like
> the
> > > labels used for deferred code generation could also run into this. 
Perhaps
> > the
> > > tests just don't exercise that scenario?
> > 
> > The more I think about it the less keen I am to be storing this as a label
in
> > the constant pool builder object. Could you just do the same as on Arm
instead
> > (getting the offset from the code object and adding that to the code target
> > address)? 
> 
> Loading the offset from the code object is problematic on PPC due to the lack
of
> pc-relative addressing.

OK.

> I disagree that this scenario or the builder's use of a label is inherently
bad.
>  As far as I can tell, it's no different than any other use of a label prior
to
> binding it.  If anything, the problem is that the Label destructor's DCHECK is
> invalid for cases such as this where the compilation was aborted and the code
> will be discarded.

Ok. How about adding a masm()->AbortConstantPoolBuilding() (just for PPC) which
explicitly binds the label to make it obvious why this is happening (and avoid
emitting the constant pool when assembly failed, which seems like it might go
wrong).

[MTBrandyberry, 2015-06-02 16:55:31]

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
File src/ppc/lithium-codegen-ppc.cc (right):

https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure in
~Label()
On 2015/06/02 16:40:12, rmcilroy wrote:
> On 2015/06/02 15:36:10, mtbrandyberry wrote:
> > On 2015/06/02 13:57:21, rmcilroy wrote:
> > > On 2015/06/01 21:01:30, mtbrandyberry wrote:
> > > > On 2015/06/01 09:52:09, rmcilroy wrote:
> > > > > Won't this cause issues in non-debug code if the label is getting
> deleted
> > > > before
> > > > > the constant pool is emitted?
> > > > 
> > > > Not quite.  The label gets deleted upon destruction of the assembler
> object.
> > 
> > > > The issue here is that the embedded constant pool label may be linked to
> in
> > > the
> > > > prologue, but never bound since this path aborts the compilation prior
to
> > > > reaching GetCode (where the constant pool would have been emitted).
> > > > 
> > > > If there is a better way to work around  this, let me know.  It seems
like
> > the
> > > > labels used for deferred code generation could also run into this. 
> Perhaps
> > > the
> > > > tests just don't exercise that scenario?
> > > 
> > > The more I think about it the less keen I am to be storing this as a label
> in
> > > the constant pool builder object. Could you just do the same as on Arm
> instead
> > > (getting the offset from the code object and adding that to the code
target
> > > address)? 
> > 
> > Loading the offset from the code object is problematic on PPC due to the
lack
> of
> > pc-relative addressing.
> 
> OK.
> 
> > I disagree that this scenario or the builder's use of a label is inherently
> bad.
> >  As far as I can tell, it's no different than any other use of a label prior
> to
> > binding it.  If anything, the problem is that the Label destructor's DCHECK
is
> > invalid for cases such as this where the compilation was aborted and the
code
> > will be discarded.
> 
> Ok. How about adding a masm()->AbortConstantPoolBuilding() (just for PPC)
which
> explicitly binds the label to make it obvious why this is happening (and avoid
> emitting the constant pool when assembly failed, which seems like it might go
> wrong).

sgtm

[MTBrandyberry, 2015-06-02 18:15:14]

On 2015/06/02 16:55:31, mtbrandyberry wrote:
>
https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
> File src/ppc/lithium-codegen-ppc.cc (right):
> 
>
https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen...
> src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure in
> ~Label()
> On 2015/06/02 16:40:12, rmcilroy wrote:
> > On 2015/06/02 15:36:10, mtbrandyberry wrote:
> > > On 2015/06/02 13:57:21, rmcilroy wrote:
> > > > On 2015/06/01 21:01:30, mtbrandyberry wrote:
> > > > > On 2015/06/01 09:52:09, rmcilroy wrote:
> > > > > > Won't this cause issues in non-debug code if the label is getting
> > deleted
> > > > > before
> > > > > > the constant pool is emitted?
> > > > > 
> > > > > Not quite.  The label gets deleted upon destruction of the assembler
> > object.
> > > 
> > > > > The issue here is that the embedded constant pool label may be linked
to
> > in
> > > > the
> > > > > prologue, but never bound since this path aborts the compilation prior
> to
> > > > > reaching GetCode (where the constant pool would have been emitted).
> > > > > 
> > > > > If there is a better way to work around  this, let me know.  It seems
> like
> > > the
> > > > > labels used for deferred code generation could also run into this. 
> > Perhaps
> > > > the
> > > > > tests just don't exercise that scenario?
> > > > 
> > > > The more I think about it the less keen I am to be storing this as a
label
> > in
> > > > the constant pool builder object. Could you just do the same as on Arm
> > instead
> > > > (getting the offset from the code object and adding that to the code
> target
> > > > address)? 
> > > 
> > > Loading the offset from the code object is problematic on PPC due to the
> lack
> > of
> > > pc-relative addressing.
> > 
> > OK.
> > 
> > > I disagree that this scenario or the builder's use of a label is
inherently
> > bad.
> > >  As far as I can tell, it's no different than any other use of a label
prior
> > to
> > > binding it.  If anything, the problem is that the Label destructor's
DCHECK
> is
> > > invalid for cases such as this where the compilation was aborted and the
> code
> > > will be discarded.
> > 
> > Ok. How about adding a masm()->AbortConstantPoolBuilding() (just for PPC)
> which
> > explicitly binds the label to make it obvious why this is happening (and
avoid
> > emitting the constant pool when assembly failed, which seems like it might
go
> > wrong).
> 
> sgtm

I think we're ready to commit with your OK.

[rmcilroy, 2015-06-02 18:34:21]

Yup, still lgtm if it passes on the bots (I'm assuming you've tested Arm with
embedded constant pools already since this isn't tested on the bots).

[rmcilroy, 2015-06-02 18:34:26]

Yup, still lgtm if it passes on the bots (I'm assuming you've tested Arm with
embedded constant pools already since this isn't tested on the bots).

[MTBrandyberry, 2015-06-02 18:39:07]

The CQ bit was checked by mbrandy@us.ibm.com

[commit-bot: I haz the power, 2015-06-02 18:39:31]

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

[commit-bot: I haz the power, 2015-06-02 18:43:43]

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

[commit-bot: I haz the power, 2015-06-02 18:43:45]

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/951)
  v8_linux64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/6343)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/4757)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/6252)

[MTBrandyberry, 2015-06-02 19:18:15]

The CQ bit was checked by mbrandy@us.ibm.com

[MTBrandyberry, 2015-06-02 19:18:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1131783003/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-02 19:18:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131783003/140001

[commit-bot: I haz the power, 2015-06-02 19:24:51]

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

[commit-bot: I haz the power, 2015-06-02 19:24:53]

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/953)

[MTBrandyberry, 2015-06-02 19:46:05]

The CQ bit was checked by mbrandy@us.ibm.com

[MTBrandyberry, 2015-06-02 19:46:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1131783003/#ps160001
(title: "Remove ool constant pool logic added via rebase.")

[commit-bot: I haz the power, 2015-06-02 19:46:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131783003/160001

[commit-bot: I haz the power, 2015-06-02 19:53:06]

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

[commit-bot: I haz the power, 2015-06-02 19:53:09]

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

[MTBrandyberry, 2015-06-02 20:29:26]

The CQ bit was checked by mbrandy@us.ibm.com

[MTBrandyberry, 2015-06-02 20:29:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1131783003/#ps180001
(title: "Fix compiler warnings / Mac does not like vector::cbegin()")

[commit-bot: I haz the power, 2015-06-02 20:29:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131783003/180001

[commit-bot: I haz the power, 2015-06-02 20:34:13]

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

[commit-bot: I haz the power, 2015-06-02 20:34:16]

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

[MTBrandyberry, 2015-06-02 20:41:29]

The CQ bit was checked by mbrandy@us.ibm.com

[MTBrandyberry, 2015-06-02 20:41:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1131783003/#ps200001
(title: "Fix other instance of cbegin")

[commit-bot: I haz the power, 2015-06-02 20:41:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131783003/200001

[commit-bot: I haz the power, 2015-06-02 20:56:05]

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

[commit-bot: I haz the power, 2015-06-02 20:56:06]

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

[MTBrandyberry, 2015-06-02 22:03:14]

The CQ bit was checked by mbrandy@us.ibm.com

[MTBrandyberry, 2015-06-02 22:03:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1131783003/#ps220001
(title: "Fix debug-mode Arm issue.")

[commit-bot: I haz the power, 2015-06-02 22:03:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131783003/220001

[commit-bot: I haz the power, 2015-06-02 22:50:17]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-06-02 22:50:22]

Patchset 12 (id:??) landed as
https://crrev.com/a9404029343d65f146e3443f5280c40a97e736af
Cr-Commit-Position: refs/heads/master@{#28770}

[Benedikt Meurer, 2015-06-03 03:00:51]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1155703006/ by bmeurer@chromium.org.

The reason for reverting is: Breaks Linux nosnap
cctest/test-api/FastReturnValuesWithProfiler, see
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20....