Enable constant pool support.

Enable constant pool support.

Enables constant pool support (distinct from existing OOL
feature).  The first consumer is PPC.  Our measurements
show that we get an overall boost of ~3% from this

R=danno@chromium.org, svenpanne@chromium.org, bmeurer@chromium.org, rmcilroy@chromium.org, mbrandy@us.ibm.com

BUG=

[michael_dawson, 2015-03-26 15:38:45]

New review for constant pool support, distinct from existing OOL support

[michael_dawson, 2015-04-02 17:11:14]

On 2015/03/26 15:38:45, michael_dawson wrote:
> New review for constant pool support, distinct from existing OOL support

Any comments/questions ? Just wondering how we get the discussion started

[rmcilroy, 2015-04-08 12:38:55]

Apologies for the delay - we were working out what we want to do with the
current OOL constant pool implementation.

The high level feedback is that we don't want to have yet another separate
configuration for constant pools, however we would be OK with supporting an
embedded constant pool and replacing the current support for OOL constant pool
in Arm with an approach which is similar to what you want to do here for PPC
(i.e., embedded at the end of the code object, with ConstantPoolArray objects
going away).

To move this forward, the plan would be to ensure that the PPC and Arm backends
use the same approach for embedded constant pools (in the case of Arm this
support would still be behind a disabled-by-default flag) and remove all
references to FLAG_enable_ool_constant_pool and replace them (where appropriate)
with FLAG_enable_embedded_constant_pool (and rip out any OOL specific code along
with the ConstantPoolArray objects).  We don't have the resources to make the
Arm changes ourselves - if you could make the changes to keep Arm working with
an embedded constant pool as part of this change that would be great (and also a
good verification that the approach is truly cross-platform), however if this is
not possible for you then we would appreciate that the Arm backend moves in the
direction of the embedded constant pool and support for the embedded constant
pool could be added with relatively minimal modifications.

I realize this leads to a fair amount more work on your part in order to get
this in, but given the number of architectures we now support in V8 it is
important that we have as few 'arch specific' code paths in the
architecture-independent code as possible in order to keep things maintainable
and testable.

I have a few more comments below.

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

https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:125: FinishCode(masm());
Don't move this - instead insert a separate step which emits the embedded
constant pool.

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

https://codereview.chromium.org/1030353003/diff/1/src/frames.cc#oldcode326
src/frames.cc:326: }
I'm not sure why you are making this change here, could you explain?

https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h
File src/ic/ic.h (right):

https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h#newcode291
src/ic/ic.h:291: };
let's not make this a union. I'm not sure that storing this as a raw address is
OK - what if a GC happens in the middle of the IC handling and the code object
movede, then this address will point to the wrong location.

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

https://codereview.chromium.org/1030353003/diff/1/src/objects.h#newcode5628
src/objects.h:5628: static const int kOOLConstantPoolOffset = kPrologueOffset +
kIntSize;
Let's just have a single field here which points to the embedded constant pool
(since we are removing the OOL constant pool support) - this should avoid you
having to do the extra tristate things below.

https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h
File src/ppc/assembler-ppc-inl.h (right):

https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h...
src/ppc/assembler-ppc-inl.h:574: Memory::Address_at(pc) = target;
What is this branch for? It looks like pc is pointing to the constant pool entry
- we shouldn't be passing this into set_target_address_at.

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

https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
src/ppc/macro-assembler-ppc.cc:686: } else {
I think these should be two separate functions (with and without the base
regisiter)

https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
src/ppc/macro-assembler-ppc.cc:700: LoadOwnConstantPoolPointerRegister(ip,
-prologue_offset);
Where does ip get set to the prologue address?

https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
src/ppc/macro-assembler-ppc.cc:737: LoadOwnConstantPoolPointerRegister(ip,
-prologue_offset);
ditto

[MTBrandyberry, 2015-04-16 15:56:39]

On 2015/04/08 12:38:55, rmcilroy wrote:
> Apologies for the delay - we were working out what we want to do with the
> current OOL constant pool implementation.
> 
> The high level feedback is that we don't want to have yet another separate
> configuration for constant pools, however we would be OK with supporting an
> embedded constant pool and replacing the current support for OOL constant pool
> in Arm with an approach which is similar to what you want to do here for PPC
> (i.e., embedded at the end of the code object, with ConstantPoolArray objects
> going away).
> 
> To move this forward, the plan would be to ensure that the PPC and Arm
backends
> use the same approach for embedded constant pools (in the case of Arm this
> support would still be behind a disabled-by-default flag) and remove all
> references to FLAG_enable_ool_constant_pool and replace them (where
appropriate)
> with FLAG_enable_embedded_constant_pool (and rip out any OOL specific code
along
> with the ConstantPoolArray objects).  We don't have the resources to make the
> Arm changes ourselves - if you could make the changes to keep Arm working with
> an embedded constant pool as part of this change that would be great (and also
a
> good verification that the approach is truly cross-platform), however if this
is
> not possible for you then we would appreciate that the Arm backend moves in
the
> direction of the embedded constant pool and support for the embedded constant
> pool could be added with relatively minimal modifications.
> 
> I realize this leads to a fair amount more work on your part in order to get
> this in, but given the number of architectures we now support in V8 it is
> important that we have as few 'arch specific' code paths in the
> architecture-independent code as possible in order to keep things maintainable
> and testable.
> 
> I have a few more comments below.
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator.cc
> File src/compiler/code-generator.cc (right):
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator...
> src/compiler/code-generator.cc:125: FinishCode(masm());
> Don't move this - instead insert a separate step which emits the embedded
> constant pool.
> 
> https://codereview.chromium.org/1030353003/diff/1/src/frames.cc
> File src/frames.cc (left):
> 
> https://codereview.chromium.org/1030353003/diff/1/src/frames.cc#oldcode326
> src/frames.cc:326: }
> I'm not sure why you are making this change here, could you explain?
> 
> https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h
> File src/ic/ic.h (right):
> 
> https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h#newcode291
> src/ic/ic.h:291: };
> let's not make this a union. I'm not sure that storing this as a raw address
is
> OK - what if a GC happens in the middle of the IC handling and the code object
> movede, then this address will point to the wrong location.
> 
> https://codereview.chromium.org/1030353003/diff/1/src/objects.h
> File src/objects.h (right):
> 
> https://codereview.chromium.org/1030353003/diff/1/src/objects.h#newcode5628
> src/objects.h:5628: static const int kOOLConstantPoolOffset = kPrologueOffset
+
> kIntSize;
> Let's just have a single field here which points to the embedded constant pool
> (since we are removing the OOL constant pool support) - this should avoid you
> having to do the extra tristate things below.
> 
> https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h
> File src/ppc/assembler-ppc-inl.h (right):
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h...
> src/ppc/assembler-ppc-inl.h:574: Memory::Address_at(pc) = target;
> What is this branch for? It looks like pc is pointing to the constant pool
entry
> - we shouldn't be passing this into set_target_address_at.
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc.cc
> File src/ppc/macro-assembler-ppc.cc (right):
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> src/ppc/macro-assembler-ppc.cc:686: } else {
> I think these should be two separate functions (with and without the base
> regisiter)
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> src/ppc/macro-assembler-ppc.cc:700: LoadOwnConstantPoolPointerRegister(ip,
> -prologue_offset);
> Where does ip get set to the prologue address?
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> src/ppc/macro-assembler-ppc.cc:737: LoadOwnConstantPoolPointerRegister(ip,
> -prologue_offset);
> ditto

Thanks for the review.  I will look into replacing the OOL constant pool for
Arm.  The target would be 32-bit Arm only, correct?

[rmcilroy, 2015-04-16 16:06:19]

On 2015/04/16 15:56:39, mtbrandyberry wrote:
> On 2015/04/08 12:38:55, rmcilroy wrote:
> > Apologies for the delay - we were working out what we want to do with the
> > current OOL constant pool implementation.
> > 
> > The high level feedback is that we don't want to have yet another separate
> > configuration for constant pools, however we would be OK with supporting an
> > embedded constant pool and replacing the current support for OOL constant
pool
> > in Arm with an approach which is similar to what you want to do here for PPC
> > (i.e., embedded at the end of the code object, with ConstantPoolArray
objects
> > going away).
> > 
> > To move this forward, the plan would be to ensure that the PPC and Arm
> backends
> > use the same approach for embedded constant pools (in the case of Arm this
> > support would still be behind a disabled-by-default flag) and remove all
> > references to FLAG_enable_ool_constant_pool and replace them (where
> appropriate)
> > with FLAG_enable_embedded_constant_pool (and rip out any OOL specific code
> along
> > with the ConstantPoolArray objects).  We don't have the resources to make
the
> > Arm changes ourselves - if you could make the changes to keep Arm working
with
> > an embedded constant pool as part of this change that would be great (and
also
> a
> > good verification that the approach is truly cross-platform), however if
this
> is
> > not possible for you then we would appreciate that the Arm backend moves in
> the
> > direction of the embedded constant pool and support for the embedded
constant
> > pool could be added with relatively minimal modifications.
> > 
> > I realize this leads to a fair amount more work on your part in order to get
> > this in, but given the number of architectures we now support in V8 it is
> > important that we have as few 'arch specific' code paths in the
> > architecture-independent code as possible in order to keep things
maintainable
> > and testable.
> > 
> > I have a few more comments below.
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator.cc
> > File src/compiler/code-generator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator...
> > src/compiler/code-generator.cc:125: FinishCode(masm());
> > Don't move this - instead insert a separate step which emits the embedded
> > constant pool.
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/frames.cc
> > File src/frames.cc (left):
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/frames.cc#oldcode326
> > src/frames.cc:326: }
> > I'm not sure why you are making this change here, could you explain?
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h
> > File src/ic/ic.h (right):
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h#newcode291
> > src/ic/ic.h:291: };
> > let's not make this a union. I'm not sure that storing this as a raw address
> is
> > OK - what if a GC happens in the middle of the IC handling and the code
object
> > movede, then this address will point to the wrong location.
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/objects.h
> > File src/objects.h (right):
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/objects.h#newcode5628
> > src/objects.h:5628: static const int kOOLConstantPoolOffset =
kPrologueOffset
> +
> > kIntSize;
> > Let's just have a single field here which points to the embedded constant
pool
> > (since we are removing the OOL constant pool support) - this should avoid
you
> > having to do the extra tristate things below.
> > 
> >
https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h
> > File src/ppc/assembler-ppc-inl.h (right):
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h...
> > src/ppc/assembler-ppc-inl.h:574: Memory::Address_at(pc) = target;
> > What is this branch for? It looks like pc is pointing to the constant pool
> entry
> > - we shouldn't be passing this into set_target_address_at.
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc.cc
> > File src/ppc/macro-assembler-ppc.cc (right):
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > src/ppc/macro-assembler-ppc.cc:686: } else {
> > I think these should be two separate functions (with and without the base
> > regisiter)
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > src/ppc/macro-assembler-ppc.cc:700: LoadOwnConstantPoolPointerRegister(ip,
> > -prologue_offset);
> > Where does ip get set to the prologue address?
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > src/ppc/macro-assembler-ppc.cc:737: LoadOwnConstantPoolPointerRegister(ip,
> > -prologue_offset);
> > ditto
> 
> Thanks for the review.  I will look into replacing the OOL constant pool for
> Arm.  The target would be 32-bit Arm only, correct?

Yes only 32-bit Arm (I'm hoping the changes would be fairly in the actual arm
backend compared to the arch-independent and ppc code).  Thanks for doing this!

[MTBrandyberry, 2015-04-30 18:59:48]

On 2015/04/16 16:06:19, rmcilroy (OOO till 18th May) wrote:
> On 2015/04/16 15:56:39, mtbrandyberry wrote:
> > On 2015/04/08 12:38:55, rmcilroy wrote:
> > > Apologies for the delay - we were working out what we want to do with the
> > > current OOL constant pool implementation.
> > > 
> > > The high level feedback is that we don't want to have yet another separate
> > > configuration for constant pools, however we would be OK with supporting
an
> > > embedded constant pool and replacing the current support for OOL constant
> pool
> > > in Arm with an approach which is similar to what you want to do here for
PPC
> > > (i.e., embedded at the end of the code object, with ConstantPoolArray
> objects
> > > going away).
> > > 
> > > To move this forward, the plan would be to ensure that the PPC and Arm
> > backends
> > > use the same approach for embedded constant pools (in the case of Arm this
> > > support would still be behind a disabled-by-default flag) and remove all
> > > references to FLAG_enable_ool_constant_pool and replace them (where
> > appropriate)
> > > with FLAG_enable_embedded_constant_pool (and rip out any OOL specific code
> > along
> > > with the ConstantPoolArray objects).  We don't have the resources to make
> the
> > > Arm changes ourselves - if you could make the changes to keep Arm working
> with
> > > an embedded constant pool as part of this change that would be great (and
> also
> > a
> > > good verification that the approach is truly cross-platform), however if
> this
> > is
> > > not possible for you then we would appreciate that the Arm backend moves
in
> > the
> > > direction of the embedded constant pool and support for the embedded
> constant
> > > pool could be added with relatively minimal modifications.
> > > 
> > > I realize this leads to a fair amount more work on your part in order to
get
> > > this in, but given the number of architectures we now support in V8 it is
> > > important that we have as few 'arch specific' code paths in the
> > > architecture-independent code as possible in order to keep things
> maintainable
> > > and testable.
> > > 
> > > I have a few more comments below.
> > > 
> > >
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator.cc
> > > File src/compiler/code-generator.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator...
> > > src/compiler/code-generator.cc:125: FinishCode(masm());
> > > Don't move this - instead insert a separate step which emits the embedded
> > > constant pool.
> > > 
> > > https://codereview.chromium.org/1030353003/diff/1/src/frames.cc
> > > File src/frames.cc (left):
> > > 
> > > https://codereview.chromium.org/1030353003/diff/1/src/frames.cc#oldcode326
> > > src/frames.cc:326: }
> > > I'm not sure why you are making this change here, could you explain?
> > > 
> > > https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h
> > > File src/ic/ic.h (right):
> > > 
> > > https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h#newcode291
> > > src/ic/ic.h:291: };
> > > let's not make this a union. I'm not sure that storing this as a raw
address
> > is
> > > OK - what if a GC happens in the middle of the IC handling and the code
> object
> > > movede, then this address will point to the wrong location.
> > > 
> > > https://codereview.chromium.org/1030353003/diff/1/src/objects.h
> > > File src/objects.h (right):
> > > 
> > >
https://codereview.chromium.org/1030353003/diff/1/src/objects.h#newcode5628
> > > src/objects.h:5628: static const int kOOLConstantPoolOffset =
> kPrologueOffset
> > +
> > > kIntSize;
> > > Let's just have a single field here which points to the embedded constant
> pool
> > > (since we are removing the OOL constant pool support) - this should avoid
> you
> > > having to do the extra tristate things below.
> > > 
> > >
> https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h
> > > File src/ppc/assembler-ppc-inl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h...
> > > src/ppc/assembler-ppc-inl.h:574: Memory::Address_at(pc) = target;
> > > What is this branch for? It looks like pc is pointing to the constant pool
> > entry
> > > - we shouldn't be passing this into set_target_address_at.
> > > 
> > >
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc.cc
> > > File src/ppc/macro-assembler-ppc.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > > src/ppc/macro-assembler-ppc.cc:686: } else {
> > > I think these should be two separate functions (with and without the base
> > > regisiter)
> > > 
> > >
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > > src/ppc/macro-assembler-ppc.cc:700: LoadOwnConstantPoolPointerRegister(ip,
> > > -prologue_offset);
> > > Where does ip get set to the prologue address?
> > > 
> > >
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > > src/ppc/macro-assembler-ppc.cc:737: LoadOwnConstantPoolPointerRegister(ip,
> > > -prologue_offset);
> > > ditto
> > 
> > Thanks for the review.  I will look into replacing the OOL constant pool for
> > Arm.  The target would be 32-bit Arm only, correct?
> 
> Yes only 32-bit Arm (I'm hoping the changes would be fairly in the actual arm
> backend compared to the arch-independent and ppc code).  Thanks for doing
this!

Update: picking this up again.  Should hopefully have something out for review
next week.

[rmcilroy, 2015-05-01 09:39:20]

> Update: picking this up again.  Should hopefully have something out for review
> next week.

Great, thanks! Just to let you know, I'm going to be OOO until the 18th May so
won't be able to review it until then, but I'll take a look as soon as I get
back (if someone else hasn't already reviewed it before then).

[MTBrandyberry, 2015-05-07 20:38:33]

I'm getting close to resubmitting this as discussed.  See responses to prior
review comments.

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

https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator...
src/compiler/code-generator.cc:125: FinishCode(masm());
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> Don't move this - instead insert a separate step which emits the embedded
> constant pool.

Acknowledged.

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

https://codereview.chromium.org/1030353003/diff/1/src/frames.cc#oldcode326
src/frames.cc:326: }
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> I'm not sure why you are making this change here, could you explain?

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

https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h
File src/ic/ic.h (right):

https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h#newcode291
src/ic/ic.h:291: };
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> let's not make this a union. I'm not sure that storing this as a raw address
is
> OK - what if a GC happens in the middle of the IC handling and the code object
> movede, then this address will point to the wrong location.  

This address is the stack slot holding the constant pool pointer -- not the
constant pool pointer itself (providing a level of indirection just like
pc_address_).  I will make this more clear.

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

https://codereview.chromium.org/1030353003/diff/1/src/objects.h#newcode5628
src/objects.h:5628: static const int kOOLConstantPoolOffset = kPrologueOffset +
kIntSize;
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> Let's just have a single field here which points to the embedded constant pool
> (since we are removing the OOL constant pool support) - this should avoid you
> having to do the extra tristate things below.

Acknowledged.

https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h
File src/ppc/assembler-ppc-inl.h (right):

https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h...
src/ppc/assembler-ppc-inl.h:574: Memory::Address_at(pc) = target;
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> What is this branch for? It looks like pc is pointing to the constant pool
entry
> - we shouldn't be passing this into set_target_address_at.

I was experimenting at one point with associating relocations with the constant
pool entry itself rather than the instruction loading the entry.  I will remove.

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

https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
src/ppc/macro-assembler-ppc.cc:686: } else {
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> I think these should be two separate functions (with and without the base
> regisiter)

Acknowledged.

https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
src/ppc/macro-assembler-ppc.cc:700: LoadOwnConstantPoolPointerRegister(ip,
-prologue_offset);
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> Where does ip get set to the prologue address?

All calls to JS code are done via the ip register.  Any instructions prior to
the prologue are adjusted for -- see LCodeGen::GeneratePrologue.

https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
src/ppc/macro-assembler-ppc.cc:737: LoadOwnConstantPoolPointerRegister(ip,
-prologue_offset);
On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> ditto

see above.

[MTBrandyberry, 2015-05-08 03:50:41]

On 2015/05/07 20:38:33, mtbrandyberry wrote:
> I'm getting close to resubmitting this as discussed.  See responses to prior
> review comments.
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator.cc
> File src/compiler/code-generator.cc (right):
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator...
> src/compiler/code-generator.cc:125: FinishCode(masm());
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > Don't move this - instead insert a separate step which emits the embedded
> > constant pool.
> 
> Acknowledged.
> 
> https://codereview.chromium.org/1030353003/diff/1/src/frames.cc
> File src/frames.cc (left):
> 
> https://codereview.chromium.org/1030353003/diff/1/src/frames.cc#oldcode326
> src/frames.cc:326: }
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > I'm not sure why you are making this change here, could you explain?
> 
> By definition, the an exit frame's pc_address is above sp (see
> ExitFrame::FillState).  Thus, this check always fails.
> 
> https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h
> File src/ic/ic.h (right):
> 
> https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h#newcode291
> src/ic/ic.h:291: };
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > let's not make this a union. I'm not sure that storing this as a raw address
> is
> > OK - what if a GC happens in the middle of the IC handling and the code
object
> > movede, then this address will point to the wrong location.  
> 
> This address is the stack slot holding the constant pool pointer -- not the
> constant pool pointer itself (providing a level of indirection just like
> pc_address_).  I will make this more clear.
> 
> https://codereview.chromium.org/1030353003/diff/1/src/objects.h
> File src/objects.h (right):
> 
> https://codereview.chromium.org/1030353003/diff/1/src/objects.h#newcode5628
> src/objects.h:5628: static const int kOOLConstantPoolOffset = kPrologueOffset
+
> kIntSize;
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > Let's just have a single field here which points to the embedded constant
pool
> > (since we are removing the OOL constant pool support) - this should avoid
you
> > having to do the extra tristate things below.
> 
> Acknowledged.
> 
> https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h
> File src/ppc/assembler-ppc-inl.h (right):
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h...
> src/ppc/assembler-ppc-inl.h:574: Memory::Address_at(pc) = target;
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > What is this branch for? It looks like pc is pointing to the constant pool
> entry
> > - we shouldn't be passing this into set_target_address_at.
> 
> I was experimenting at one point with associating relocations with the
constant
> pool entry itself rather than the instruction loading the entry.  I will
remove.
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc.cc
> File src/ppc/macro-assembler-ppc.cc (right):
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> src/ppc/macro-assembler-ppc.cc:686: } else {
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > I think these should be two separate functions (with and without the base
> > regisiter)
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> src/ppc/macro-assembler-ppc.cc:700: LoadOwnConstantPoolPointerRegister(ip,
> -prologue_offset);
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > Where does ip get set to the prologue address?
> 
> All calls to JS code are done via the ip register.  Any instructions prior to
> the prologue are adjusted for -- see LCodeGen::GeneratePrologue.
> 
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> src/ppc/macro-assembler-ppc.cc:737: LoadOwnConstantPoolPointerRegister(ip,
> -prologue_offset);
> On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > ditto
> 
> see above.

Resubmitted via new issue: https://codereview.chromium.org/1131783003/

[michael_dawson, 2015-05-08 14:05:38]

On 2015/05/08 03:50:41, mtbrandyberry wrote:
> On 2015/05/07 20:38:33, mtbrandyberry wrote:
> > I'm getting close to resubmitting this as discussed.  See responses to prior
> > review comments.
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator.cc
> > File src/compiler/code-generator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/compiler/code-generator...
> > src/compiler/code-generator.cc:125: FinishCode(masm());
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > Don't move this - instead insert a separate step which emits the embedded
> > > constant pool.
> > 
> > Acknowledged.
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/frames.cc
> > File src/frames.cc (left):
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/frames.cc#oldcode326
> > src/frames.cc:326: }
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > I'm not sure why you are making this change here, could you explain?
> > 
> > By definition, the an exit frame's pc_address is above sp (see
> > ExitFrame::FillState).  Thus, this check always fails.
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h
> > File src/ic/ic.h (right):
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/ic/ic.h#newcode291
> > src/ic/ic.h:291: };
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > let's not make this a union. I'm not sure that storing this as a raw
address
> > is
> > > OK - what if a GC happens in the middle of the IC handling and the code
> object
> > > movede, then this address will point to the wrong location.  
> > 
> > This address is the stack slot holding the constant pool pointer -- not the
> > constant pool pointer itself (providing a level of indirection just like
> > pc_address_).  I will make this more clear.
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/objects.h
> > File src/objects.h (right):
> > 
> > https://codereview.chromium.org/1030353003/diff/1/src/objects.h#newcode5628
> > src/objects.h:5628: static const int kOOLConstantPoolOffset =
kPrologueOffset
> +
> > kIntSize;
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > Let's just have a single field here which points to the embedded constant
> pool
> > > (since we are removing the OOL constant pool support) - this should avoid
> you
> > > having to do the extra tristate things below.
> > 
> > Acknowledged.
> > 
> >
https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h
> > File src/ppc/assembler-ppc-inl.h (right):
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/assembler-ppc-inl.h...
> > src/ppc/assembler-ppc-inl.h:574: Memory::Address_at(pc) = target;
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > What is this branch for? It looks like pc is pointing to the constant pool
> > entry
> > > - we shouldn't be passing this into set_target_address_at.
> > 
> > I was experimenting at one point with associating relocations with the
> constant
> > pool entry itself rather than the instruction loading the entry.  I will
> remove.
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc.cc
> > File src/ppc/macro-assembler-ppc.cc (right):
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > src/ppc/macro-assembler-ppc.cc:686: } else {
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > I think these should be two separate functions (with and without the base
> > > regisiter)
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > src/ppc/macro-assembler-ppc.cc:700: LoadOwnConstantPoolPointerRegister(ip,
> > -prologue_offset);
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > Where does ip get set to the prologue address?
> > 
> > All calls to JS code are done via the ip register.  Any instructions prior
to
> > the prologue are adjusted for -- see LCodeGen::GeneratePrologue.
> > 
> >
>
https://codereview.chromium.org/1030353003/diff/1/src/ppc/macro-assembler-ppc...
> > src/ppc/macro-assembler-ppc.cc:737: LoadOwnConstantPoolPointerRegister(ip,
> > -prologue_offset);
> > On 2015/04/08 12:38:55, rmcilroy (OOO till 18th May) wrote:
> > > ditto
> > 
> > see above.
> 
> Resubmitted via new issue: https://codereview.chromium.org/1131783003/

closing as there is a new issue to cover