Add out-of-line constant pool support to Arm.

Add out-of-line constant pool support to Arm.

This CL adds out-of-line constant pool support to Arm.  A ConstantPoolBuilder
object is introduced to manage building of the ConstantPoolArray for a given
code object.

This CL depends on the following CLs landing first:
  https://codereview.chromium.org/138503002/
  https://codereview.chromium.org/179813005/
  https://codereview.chromium.org/183553003/
  https://codereview.chromium.org/183803022/
  https://codereview.chromium.org/183883011/
  https://codereview.chromium.org/186733006/
  https://codereview.chromium.org/188063002/
  https://codereview.chromium.org/190793002/
  https://codereview.chromium.org/190823002/
  https://codereview.chromium.org/190833002/
  https://codereview.chromium.org/190883002/

R=ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20051

[rmcilroy, 2014-03-07 18:50:24]

This CL introduces the ool constant pool on arm support.  I've tried to split up
the massive patch-set into a set of manageable CLs for reviewing (the list of
dependent CLs is in the CL description).  I have also uploaded the complete
patch-set in case you want to see the context in:
  https://codereview.chromium.org/191313002

There are still a couple of failures in arm.debug.check if
FLAG_ool_constant_pool is enabled - mainly to do with: 
  - Call sizes not always being the expected size (e.g., if the constant pool is
not available / is full).
  - Embedded objects not being treated weakly in optimized code by the
VisitConstantPoolArray gc visitor.
I will look into addressing these next.

PTAL

[Rodolph Perfetta (ARM), 2014-03-12 14:46:19]

just a few nits, otherwise looks good to me.

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

https://codereview.chromium.org/191233003/diff/10001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:1165: }
nit: indent

https://codereview.chromium.org/191233003/diff/10001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/191233003/diff/10001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:970: __ ldr(pp, MemOperand(r0, Code::kConstantPoolOffset
- kHeapObjectTag));
use FieldMemOperand so you don't need to substract the tag.

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

https://codereview.chromium.org/191233003/diff/10001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:932: set_constant_pool_available(true);
duplication

https://codereview.chromium.org/191233003/diff/10001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:3577: const int32_t kPCRegOffset = 2 *
kPointerSize;
use kPCReadOffset insetad

[rmcilroy, 2014-03-17 18:31:34]

Thanks for the review Rodolph!

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

https://codereview.chromium.org/191233003/diff/10001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:1165: }
On 2014/03/12 14:46:20, Rodolph Perfetta (ARM) wrote:
> nit: indent

Done.

https://codereview.chromium.org/191233003/diff/10001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/191233003/diff/10001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:970: __ ldr(pp, MemOperand(r0, Code::kConstantPoolOffset
- kHeapObjectTag));
On 2014/03/12 14:46:20, Rodolph Perfetta (ARM) wrote:
> use FieldMemOperand so you don't need to substract the tag.

Done (also for surrounding calls).

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

https://codereview.chromium.org/191233003/diff/10001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:932: set_constant_pool_available(true);
On 2014/03/12 14:46:20, Rodolph Perfetta (ARM) wrote:
> duplication

Thanks (merge errors...)

https://codereview.chromium.org/191233003/diff/10001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:3577: const int32_t kPCRegOffset = 2 *
kPointerSize;
On 2014/03/12 14:46:20, Rodolph Perfetta (ARM) wrote:
> use kPCReadOffset insetad

Done.

[ulan, 2014-03-18 12:27:51]

Looks good! Few comments:

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

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:296: // The deserializer needs to know whether a
pointer is specially coded.  Bein
"Being"

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:1102: // If there is no constant pool available, we
must use an mov immediate.
Maybe ASSERT(CpuFeatures::IsSupported(MOVW_MOVT_IMMEDIATE_LOADS)) here?

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:3313: if (FLAG_enable_ool_constant_pool) return;
If FLAG_enable_ool_constant_pool then this function should be a no-op, right?
Maybe assert:
FLAG_enable_ool_constant_pool => num_pending_32_bit_reloc_info_ == 0
FLAG_enable_ool_constant_pool => num_pending_64_bit_reloc_info_ == 0

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:3336: if (FLAG_enable_ool_constant_pool) return;
Same as above.

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:3636: int new_buffer_size = buffer_size_ * 2;
Since we already depend on STL, I'd suggest to use std::vector here.

https://codereview.chromium.org/191233003/diff/30001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/191233003/diff/30001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:967: ConstantPoolUnavailableScope
constant_pool_unavailable(masm);
As discussed in one of the previous CLs, let's make the extent of the scope more
clear by putting it in { }.

https://codereview.chromium.org/191233003/diff/30001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/191233003/diff/30001/src/heap.cc#newcode4171
src/heap.cc:4171: maybe_result = CopyConstantPoolArray(code->constant_pool());
Since const pool is smaller then code, it is probably better to allocate and
copy const pool before allocating and copying the code. I think this would make
the amount of wasted work smaller if there is an allocation failure.

https://codereview.chromium.org/191233003/diff/30001/src/heap.cc#newcode4228
src/heap.cc:4228: Object* constant_pool_copy;
The same as above.

[rmcilroy, 2014-03-18 12:31:39]

Cleaned up constant pool allocation.  PTAL.

[rmcilroy, 2014-03-18 15:14:35]

Thanks Ulan. Comments addressed. PTAL.

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

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:296: // The deserializer needs to know whether a
pointer is specially coded.  Bein
On 2014/03/18 12:27:51, ulan wrote:
> "Being"

Done.

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:1102: // If there is no constant pool available, we
must use an mov immediate.
On 2014/03/18 12:27:51, ulan wrote:
> Maybe ASSERT(CpuFeatures::IsSupported(MOVW_MOVT_IMMEDIATE_LOADS)) here?

What we actually want is IsSupported(ARMv7).  MOVW_MOVT_IMMEDIATE_LOADS is only
true on qualcomm even to force movw_movt usage (confusing name), but all armv7
chips support it.

Done.

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:3313: if (FLAG_enable_ool_constant_pool) return;
On 2014/03/18 12:27:51, ulan wrote:
> If FLAG_enable_ool_constant_pool then this function should be a no-op, right?
> Maybe assert:
> FLAG_enable_ool_constant_pool => num_pending_32_bit_reloc_info_ == 0
> FLAG_enable_ool_constant_pool => num_pending_64_bit_reloc_info_ == 0

Done.

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:3336: if (FLAG_enable_ool_constant_pool) return;
On 2014/03/18 12:27:51, ulan wrote:
> Same as above.

Done.

https://codereview.chromium.org/191233003/diff/30001/src/arm/assembler-arm.cc...
src/arm/assembler-arm.cc:3636: int new_buffer_size = buffer_size_ * 2;
On 2014/03/18 12:27:51, ulan wrote:
> Since we already depend on STL, I'd suggest to use std::vector here.

Done.

https://codereview.chromium.org/191233003/diff/30001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/191233003/diff/30001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:967: ConstantPoolUnavailableScope
constant_pool_unavailable(masm);
On 2014/03/18 12:27:51, ulan wrote:
> As discussed in one of the previous CLs, let's make the extent of the scope
more
> clear by putting it in { }.

Done.

https://codereview.chromium.org/191233003/diff/30001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/191233003/diff/30001/src/heap.cc#newcode4171
src/heap.cc:4171: maybe_result = CopyConstantPoolArray(code->constant_pool());
On 2014/03/18 12:27:51, ulan wrote:
> Since const pool is smaller then code, it is probably better to allocate and
> copy const pool before allocating and copying the code. I think this would
make
> the amount of wasted work smaller if there is an allocation failure.

Done.

https://codereview.chromium.org/191233003/diff/30001/src/heap.cc#newcode4228
src/heap.cc:4228: Object* constant_pool_copy;
On 2014/03/18 12:27:51, ulan wrote:
> The same as above.

Done.

[ulan, 2014-03-18 16:06:31]

LGTM

[rmcilroy, 2014-03-19 09:38:31]

Committed patchset #6 manually as r20051 (presubmit successful).