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.cc#newcode125
> 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#newcode574
> 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.cc#newcode686
> 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.cc#newcode700
> 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.cc#newcode737
> 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
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.
