+ishell
Igor: Could you have a quick look at the changes in mark-compact.cc and
check
they don't break slots filtering?
Other than that looks good, with one last question in
lithium-codegen-ppc.cc and
a couple of small nits.
https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/heap/mark-compact.cc#newcode4627
src/heap/mark-compact.cc:4627: slot_type = SlotsBuffer::OBJECT_SLOT;
On 2015/05/20 22:28:22, mtbrandyberry wrote:
On 2015/05/20 14:32:11, rmcilroy wrote:
> why are these changes to add OBJECT_SLOT to the mark compact
collector
required?
> Can they be made in a different CL?
This is required.
https://chromium.googlesource.com/v8/v8/+/7ad9980d995b0381046ee8677861491ef6d234ed
effectively broke ool/embedded constant pools. I believe this
restores
functionality without violating the intent of that change.
ishell@: Could you have a quick look at this change an check it won't
cause issues with slot filtering again.
https://codereview.chromium.org/1131783003/diff/40001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):
https://codereview.chromium.org/1131783003/diff/40001/src/arm/assembler-arm.cc#newcode478
src/arm/assembler-arm.cc:478: offset = EmitEmbeddedConstantPool();
/s/offset/constant_pool_offset
https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h
File src/assembler.h (right):
https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h#newcode1154
src/assembler.h:1154: // Embedded constant pool support
ConstntPoolEntry is used by both embedded and inline constant pools now
right? Change the comment here to "Constant pool support" and add one
above ConstantPoolBuilder with "Embedded constant pool support"
https://codereview.chromium.org/1131783003/diff/40001/src/assembler.h#newcode1189
src/assembler.h:1189: friend class ConstantPoolBuilder;
is this only for setting merged_index_? If so, I would prefer to add a
set_merged_index() function rather than requiring that the classes be
friended
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()
Won't this cause issues in non-debug code if the label is getting
deleted before the constant pool is emitted?
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#newcode1
test/cctest/test-constantpool.cc:1: // Copyright 2013 the V8 project
authors. All rights reserved.
2015. Also you just need the short copyright notice now:
// Copyright 2015 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can
be
// found in the LICENSE file.
https://www.chromium.org/developers/coding-style#TOC-File-headers
https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-constantpool.cc#newcode43
test/cctest/test-constantpool.cc:43: const int kReach = 1 << kReachBits;
nit - could you add one test which has dblReach different from intReach
to check that works as intended.
https://codereview.chromium.org/1131783003/diff/40001/test/cctest/test-constantpool.cc#newcode218
test/cctest/test-constantpool.cc:218: }
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?
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.
