Out-of-line constant pool on Arm: Stage 3 - Set Constant Pool Pointer on Function Entry

Out-of-line constant pool on Arm: Stage 3 - Set Constant Pool Pointer on Function Entry

Third stage of implementing an out-of-line constant pool for Arm.  This CL adds
a ConstantPool field to Code objects and initializes the pp register on
function entry, and saves the pp register on the stack frame. The ConstantPool
object is always empty and is unused currently.

R=ulan@chromium.org

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

[rmcilroy, 2013-11-26 14:10:12]

Ulan, please review this change.  JFB, Rodolph and Benedikt, FYI (or review too
if you would like to).

[JF, 2013-11-26 17:09:55]

Could you re-upload? Only the unified diff worked, not the side-by-side, it's
much easier to read :-)

[rmcilroy, 2013-11-26 18:12:09]

On 2013/11/26 17:09:55, JF wrote:
> Could you re-upload? Only the unified diff worked, not the side-by-side, it's
> much easier to read :-)

Done, hopefully this is better.

[JF, 2013-11-26 18:43:59]

First look. As discussed, would it make more sense for pp to be callee saved?

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

https://codereview.chromium.org/88043002/diff/40001/src/arm/builtins-arm.cc#n...
src/arm/builtins-arm.cc:848: __ PushFixedFrame(r1);
Maybe I'm missing some context, but why is this needed?

https://codereview.chromium.org/88043002/diff/40001/src/arm/builtins-arm.cc#n...
src/arm/builtins-arm.cc:1361: __ stm(db_w, sp, r0.bit() | r1.bit() | r4.bit());
Can you explain why r4 is spilled twice?

[rmcilroy, 2013-11-27 17:33:55]

As discussed, I made pp callee saved.  I also changed the order of pp and cp on
the stackframe so that popping is more efficent, and in doing so, swapped the
registers which hold cp and pp (r7, r8).  PTAL.

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

https://codereview.chromium.org/88043002/diff/40001/src/arm/builtins-arm.cc#n...
src/arm/builtins-arm.cc:848: __ PushFixedFrame(r1);
On 2013/11/26 18:43:59, JF wrote:
> Maybe I'm missing some context, but why is this needed?

If code is pre-aged it will hit call this stub (rather than running it's
prologue).  Usually, the code-age stub will patch the prologue back in again,
and the stub will return to the start of the function and execute this prologue.
 In the case of the "MarkAsExecutedOnce" code stub however, the prologue code is
replaced with another marker, so we need to do the usual prologue operations
here, and jump to the point after the code-age marker.

https://codereview.chromium.org/88043002/diff/40001/src/arm/builtins-arm.cc#n...
src/arm/builtins-arm.cc:1361: __ stm(db_w, sp, r0.bit() | r1.bit() | r4.bit());
On 2013/11/26 18:43:59, JF wrote:
> Can you explain why r4 is spilled twice?

The second r4 is pushed where pp would usually be in the normal FixedFrame (We
need to push something here in order to have a proper FixedFrame, but didn't
need to push pp, and doing so would have not worked using stm due to the
register order).  However, due to my change to re-order the constant pool stack
slot, I can now push everything (including the pp) in a single stm instruction.

[Rodolph Perfetta, 2013-11-27 20:09:40]

https://codereview.chromium.org/88043002/diff/100001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/88043002/diff/100001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:1997: __ mov(r8, Operand(Smi::FromInt(marker)));
if (FLAG_enable_ool_constant_pool)

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

https://codereview.chromium.org/88043002/diff/100001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:440: int no_frame_start = masm_->pc_offset();
previously no_frame_start didn't include the mov. Now it does.

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

https://codereview.chromium.org/88043002/diff/100001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:606: ASSERT(!marker_reg.is_valid() ||
marker_reg.code() < pp.code());
marker_reg.code() < cp.code()

https://codereview.chromium.org/88043002/diff/100001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:616: ASSERT(!marker_reg.is_valid() ||
marker_reg.code() < pp.code());
ditto.

[Benedikt Meurer, 2013-11-28 07:18:28]

On 2013/11/27 17:33:55, rmcilroy wrote:
> As discussed, I made pp callee saved.  I also changed the order of pp and cp
on
> the stackframe so that popping is more efficent, and in doing so, swapped the
> registers which hold cp and pp (r7, r8).  PTAL.

Please split this swapping into a separate CL.

[rmcilroy, 2013-11-28 11:32:26]

https://codereview.chromium.org/88043002/diff/100001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/88043002/diff/100001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:1997: __ mov(r8, Operand(Smi::FromInt(marker)));
On 2013/11/27 20:09:41, Rodolph Perfetta wrote:
> if (FLAG_enable_ool_constant_pool)

Done.

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

https://codereview.chromium.org/88043002/diff/100001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:440: int no_frame_start = masm_->pc_offset();
On 2013/11/27 20:09:41, Rodolph Perfetta wrote:
> previously no_frame_start didn't include the mov. Now it does.

Yes, I wasn't sure on the implications this would have.  I've changed LeaveFrame
so that it now returns the PC offset where the frame is left.

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

https://codereview.chromium.org/88043002/diff/100001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:606: ASSERT(!marker_reg.is_valid() ||
marker_reg.code() < pp.code());
On 2013/11/27 20:09:41, Rodolph Perfetta wrote:
> marker_reg.code() < cp.code()

Good catch, thanks!  Done.

https://codereview.chromium.org/88043002/diff/100001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:616: ASSERT(!marker_reg.is_valid() ||
marker_reg.code() < pp.code());
On 2013/11/27 20:09:41, Rodolph Perfetta wrote:
> ditto.

Done.

[rmcilroy, 2013-11-28 11:56:33]

On 2013/11/28 07:18:28, Benedikt Meurer wrote:
> On 2013/11/27 17:33:55, rmcilroy wrote:
> > As discussed, I made pp callee saved.  I also changed the order of pp and cp
> on
> > the stackframe so that popping is more efficent, and in doing so, swapped
the
> > registers which hold cp and pp (r7, r8).  PTAL.
> 
> Please split this swapping into a separate CL.

Done - cp/pp swap cl is https://codereview.chromium.org/94723002/

[Rodolph Perfetta, 2013-11-28 18:35:19]

On 2013/11/28 11:56:33, rmcilroy wrote:
> On 2013/11/28 07:18:28, Benedikt Meurer wrote:
> > On 2013/11/27 17:33:55, rmcilroy wrote:
> > > As discussed, I made pp callee saved.  I also changed the order of pp and
cp
> > on
> > > the stackframe so that popping is more efficent, and in doing so, swapped
> the
> > > registers which hold cp and pp (r7, r8).  PTAL.
> > 
> > Please split this swapping into a separate CL.
> 
> Done - cp/pp swap cl is https://codereview.chromium.org/94723002/

When I set FLAG_enable_ool_constant_pool to true it crashes. Did I miss
something?

[rmcilroy, 2013-12-11 16:56:51]

> When I set FLAG_enable_ool_constant_pool to true it crashes. Did I miss
> something?

Apologies for the delay (just back from vacation).  There was a merge error in
the last rebase merge I did which caused this, sorry about that.  It should work
with the latest patch set (note, setting enable_ool_constant_pool to true will
only work on Arm), please let me know if you have any issues.

[JF, 2013-12-13 17:33:38]

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

https://codereview.chromium.org/88043002/diff/230001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:1375: __ add(sp, sp, Operand(kPointerSize));  // adjust
for receiver
Does this adjust for ool constant properly?

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

https://codereview.chromium.org/88043002/diff/230001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:434: // tool from instrumenting as we rely on the
code size here.
This comment seems to say __ shouldn't be used at least for a certain scope,
could you add a comment where that scope banning __ ends?

[rmcilroy, 2013-12-16 14:56:20]

Adding danno@ for question in full-codegen-arm.cc comments.

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

https://codereview.chromium.org/88043002/diff/230001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:1375: __ add(sp, sp, Operand(kPointerSize));  // adjust
for receiver
On 2013/12/13 17:33:39, JF wrote:
> Does this adjust for ool constant properly?

The line above: __ mov(sp, fp) drops the ool pointer slot as well as the rest of
the fixed frame.

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

https://codereview.chromium.org/88043002/diff/230001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:434: // tool from instrumenting as we rely on the
code size here.
On 2013/12/13 17:33:39, JF wrote:
> This comment seems to say __ shouldn't be used at least for a certain scope,
> could you add a comment where that scope banning __ ends?

I missed this comment.  From talking with the the original author (Soren) who
added this comment, he said he didn't think the code coverage tool was used any
longer.  

Daniel: Do you know if this is the case?

I've changing all to masm()-> to __ on this assumption.

[Sven Panne, 2013-12-17 07:23:58]

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

https://codereview.chromium.org/88043002/diff/230001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:434: // tool from instrumenting as we rely on the
code size here.
On 2013/12/16 14:56:21, rmcilroy wrote:
> [...] From talking with the the original author (Soren) who
> added this comment, he said he didn't think the code coverage tool was used
any
> longer.  
> 
> Daniel: Do you know if this is the case? [...]

I can't find any place where GENERATED_CODE_COVERAGE is #defined, so I guess
that it is untested for several years, which in turn implies that it is broken
with 99.99% probability, anyway. :-) So my gut feeling is that we should simply
remove this stuff.

[ulan, 2013-12-17 08:35:10]

Hi Ross,

Wouldn't it be better to add constant pool pointer to code object instead of
JSFunction?
JSFunctions can share the same code object. So this would save memory and would
avoid potential bugs when we replace code of a function for
optmization/deoptimization.

[rmcilroy, 2013-12-18 13:23:07]

Great idea Ulan - moving the constant pool to the Code object rather than the
JSFunction seems to work much better and should be less prone to weird bugs
during OSR. It also means that we can use the constant pool in code-stubs easily
too.

PTAL.

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

https://codereview.chromium.org/88043002/diff/230001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:434: // tool from instrumenting as we rely on the
code size here.
> I can't find any place where GENERATED_CODE_COVERAGE is #defined, so I guess
> that it is untested for several years, which in turn implies that it is broken
> with 99.99% probability, anyway. :-) So my gut feeling is that we should
simply
> remove this stuff.

Great, thanks Sven. Done.

[ulan, 2013-12-18 15:06:55]

Looking good overall. I am reviewing the details.

One part is not clear to me. In the instructions that come before loading the
constant pool register, do we use inline constant pool or disallow constant pool
accesses?

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:1360:
Operand(StandardFrameConstants::kFixedFrameSizeFromFp + kPointerSize));
This is from another change:
I wonder if usage of StandardFrameConstants::kFixedFrameSizeFromFp is correct
here. This doesn't look like a standard frame.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:166: __ LoadConstantPoolPointerRegister();
Ensure that there are no constant pool accesses before this point.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:165: if (NeedsEagerFrame()) {
If there is no eager frame, then we load constant from the inline constant pool?

https://codereview.chromium.org/88043002/diff/290001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:169: __ LoadConstantPoolPointerRegister();
Ensure that there are no constant pool accesses before this point.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:920: ldr(pp, MemOperand(pc,
Code::kConstantPoolOffset - Code::kHeaderSize -
Ensure that the offset doesn't require constant pool.

[rmcilroy, 2013-12-19 11:08:24]

> One part is not clear to me. In the instructions that come
> before loading the constant pool register, do we use inline 
> constant pool or disallow constant pool accesses?

My plan is to only allow movev/movew or mov/orr/orr instructions before the
constant pool register is setup - I think we want to get rid of the inline
constant pool entirely after the ool one is working since it adds a fair bit of
complexity to the code.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:1360:
Operand(StandardFrameConstants::kFixedFrameSizeFromFp + kPointerSize));
On 2013/12/18 15:06:56, ulan wrote:
> This is from another change:
> I wonder if usage of StandardFrameConstants::kFixedFrameSizeFromFp is correct
> here. This doesn't look like a standard frame.

So my thought process was that each stack frame always has the fixed frame slots
first, but in some frames the meaning of the stack slots changes (e.g., in this
case, the usual cp slot is replaced with the argument adaptor marker, and the
marker bit is replaced with r1).  Does this sound reasonable?

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:166: __ LoadConstantPoolPointerRegister();
On 2013/12/18 15:06:56, ulan wrote:
> Ensure that there are no constant pool accesses before this point.

I will be doing this when I start using the constant pool in the next CL (I'll
have a bool in the Assembler class specifying whether we can use the constant
pool yet, which will only be set to true after this point).

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:165: if (NeedsEagerFrame()) {
On 2013/12/18 15:06:56, ulan wrote:
> If there is no eager frame, then we load constant from the inline constant
pool?

We aren't going to have an inline constant pool any longer once the OOL one is
finished.  My plan is to have the asm generate movew/movev or mov/orr/orr
instruction sequences until it knows it can use the constant pool.  We might
want to have some way for lithium to know that it should output a lazy frame if
it knows that a bit of upcoming code would benefit from the constant pool
though.  Any thoughts here?

https://codereview.chromium.org/88043002/diff/290001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:169: __ LoadConstantPoolPointerRegister();
On 2013/12/18 15:06:56, ulan wrote:
> Ensure that there are no constant pool accesses before this point.

Ditto.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.cc:920: ldr(pp, MemOperand(pc,
Code::kConstantPoolOffset - Code::kHeaderSize -
On 2013/12/18 15:06:56, ulan wrote:
> Ensure that the offset doesn't require constant pool.

Done.

[ulan, 2013-12-19 12:54:41]

LGTM with nits.

> My plan is to only allow movev/movew or mov/orr/orr instructions before the
> constant pool register is setup - I think we want to get rid of the inline
> constant pool entirely after the ool one is working since it adds a fair bit
of
> complexity to the code.
Thanks for clarification, sounds good.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:1360:
Operand(StandardFrameConstants::kFixedFrameSizeFromFp + kPointerSize));
On 2013/12/19 11:08:25, rmcilroy wrote:
> On 2013/12/18 15:06:56, ulan wrote:
> > This is from another change:
> > I wonder if usage of StandardFrameConstants::kFixedFrameSizeFromFp is
correct
> > here. This doesn't look like a standard frame.
> 
> So my thought process was that each stack frame always has the fixed frame
slots
> first, but in some frames the meaning of the stack slots changes (e.g., in
this
> case, the usual cp slot is replaced with the argument adaptor marker, and the
> marker bit is replaced with r1).  Does this sound reasonable?
I see. Yes, this is reasonable.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:166: __ LoadConstantPoolPointerRegister();
On 2013/12/19 11:08:25, rmcilroy wrote:
> On 2013/12/18 15:06:56, ulan wrote:
> > Ensure that there are no constant pool accesses before this point.
> 
> I will be doing this when I start using the constant pool in the next CL (I'll
> have a bool in the Assembler class specifying whether we can use the constant
> pool yet, which will only be set to true after this point). 
Yep, this will work.

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:165: if (NeedsEagerFrame()) {
On 2013/12/19 11:08:25, rmcilroy wrote:
> On 2013/12/18 15:06:56, ulan wrote:
> > If there is no eager frame, then we load constant from the inline constant
> pool?
> 
> We aren't going to have an inline constant pool any longer once the OOL one is
> finished.  My plan is to have the asm generate movew/movev or mov/orr/orr
> instruction sequences until it knows it can use the constant pool.  We might
> want to have some way for lithium to know that it should output a lazy frame
if
> it knows that a bit of upcoming code would benefit from the constant pool
> though.  Any thoughts here?
Let's see how this will affect performance. Maybe movew/movev will be
sufficient. Otherwise, we need to find some way to save/restore caller's pp.

https://codereview.chromium.org/88043002/diff/310001/src/arm/macro-assembler-...
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/88043002/diff/310001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.h:429: // Push a fixed frame, consisting of lr, fp,
context, constant pool
swap "context" with "constant pool".

https://codereview.chromium.org/88043002/diff/310001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/88043002/diff/310001/src/frames.h#newcode170
src/frames.h:170: // context, function, and constant pool (if
FLAG_enable_ool_constant_pool).
Fix order of constant pool.

[rmcilroy, 2013-12-19 13:31:58]

Thanks Ulan!

Does anyone else what to chime in before I submit this?

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

https://codereview.chromium.org/88043002/diff/290001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:165: if (NeedsEagerFrame()) {
On 2013/12/19 12:54:41, ulan wrote:
> On 2013/12/19 11:08:25, rmcilroy wrote:
> > On 2013/12/18 15:06:56, ulan wrote:
> > > If there is no eager frame, then we load constant from the inline constant
> > pool?
> > 
> > We aren't going to have an inline constant pool any longer once the OOL one
is
> > finished.  My plan is to have the asm generate movew/movev or mov/orr/orr
> > instruction sequences until it knows it can use the constant pool.  We might
> > want to have some way for lithium to know that it should output a lazy frame
> if
> > it knows that a bit of upcoming code would benefit from the constant pool
> > though.  Any thoughts here?
> Let's see how this will affect performance. Maybe movew/movev will be
> sufficient. Otherwise, we need to find some way to save/restore caller's pp.

SGTM

https://codereview.chromium.org/88043002/diff/310001/src/arm/macro-assembler-...
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/88043002/diff/310001/src/arm/macro-assembler-...
src/arm/macro-assembler-arm.h:429: // Push a fixed frame, consisting of lr, fp,
context, constant pool
On 2013/12/19 12:54:41, ulan wrote:
> swap "context" with "constant pool".

Done.

https://codereview.chromium.org/88043002/diff/310001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/88043002/diff/310001/src/frames.h#newcode170
src/frames.h:170: // context, function, and constant pool (if
FLAG_enable_ool_constant_pool).
On 2013/12/19 12:54:41, ulan wrote:
> Fix order of constant pool.

Done.

[rmcilroy, 2013-12-30 11:24:17]

Committed patchset #20 manually as r18425 (presubmit successful).