lgtm once EmittedLabel is removed from the ConstantPoolBuilder (otherwise
let's
do another review round).
+Rodolph as FYI on Arm changes.
https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen-ppc.cc
File src/ppc/lithium-codegen-ppc.cc (right):
https://codereview.chromium.org/1131783003/diff/40001/src/ppc/lithium-codegen-ppc.cc#newcode57
src/ppc/lithium-codegen-ppc.cc:57: // Avoid DCHECK(!is_linked()) failure
in ~Label()
On 2015/06/01 21:01:30, mtbrandyberry wrote:
On 2015/06/01 09:52:09, rmcilroy wrote:
> Won't this cause issues in non-debug code if the label is getting
deleted
before
> the constant pool is emitted?
Not quite. The label gets deleted upon destruction of the assembler
object.
The issue here is that the embedded constant pool label may be linked
to in the
prologue, but never bound since this path aborts the compilation prior
to
reaching GetCode (where the constant pool would have been emitted).
If there is a better way to work around this, let me know. It seems
like the
labels used for deferred code generation could also run into this.
Perhaps the
tests just don't exercise that scenario?
The more I think about it the less keen I am to be storing this as a
label in the constant pool builder object. Could you just do the same as
on Arm instead (getting the offset from the code object and adding that
to the code target address)?
https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-constantpool.cc
File test/cctest/test-constantpool.cc (right):
https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-constantpool.cc#newcode218
test/cctest/test-constantpool.cc:218: }
On 2015/06/01 21:01:31, mtbrandyberry wrote:
On 2015/06/01 09:52:09, rmcilroy wrote:
> It would be nice if you could test the emitting logic as well,
though I
realize
> this would be tricky to do in a non-arch specific way. Would it be
possible to
> add some test of the emitting logic to test-assembler-ppc?
I considered this, however other than validating the expected size of
the
emitted pool, there isn't much else to assert on that adds value
beyond just
running the rest of the tests with embedded pools enabled.
I'd like to consider this further as a separate CL. PPC in general
needs better
coverage in test-assembler-ppc.
OK fair enough.
https://codereview.chromium.org/1131783003/diff/60001/test/cctest/test-constantpool.cc
File test/cctest/test-constantpool.cc (right):
https://codereview.chromium.org/1131783003/diff/60001/test/cctest/test-constantpool.cc#newcode1
test/cctest/test-constantpool.cc:1: // Copyright 2013 the V8 project
authors. All rights reserved.
Still need to update date to 2015.
https://codereview.chromium.org/1131783003/
--
--
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.
