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.cc#newcode125
> > 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#newcode574
> > 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.cc#newcode686
> > 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.cc#newcode700
> > 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.cc#newcode737
> > 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.
https://codereview.chromium.org/1030353003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
