My responses below. Highlights are that I agree with most of the comments
and
have provided clarifications for a small number. I'll work on providing the
unittest now (but wanted to address the others and get the re-review
started in
parallel).
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#newcode452
src/arm/assembler-arm.cc:452: constant_pool_builder_(12, 10),
On 2015/05/20 14:32:10, rmcilroy wrote:
make the '12' and '10' constants defined in constants-arm.h
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#newcode1083
src/arm/assembler-arm.cc:1083: // An extended constant pool load.
On 2015/05/20 14:32:10, rmcilroy wrote:
update comment.
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#newcode3592
src/arm/assembler-arm.cc:3592: CheckBuffer();
On 2015/05/20 14:32:10, rmcilroy wrote:
Please add the num_pending_xx_bit_constants_ == 0 DCHECKs here too.
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#newcode3894
src/arm/assembler-arm.cc:3894: void Assembler::SetConstantPoolOffset(int
pos, int offset,
On 2015/05/20 14:32:10, rmcilroy wrote:
I think this would be clearer as something like
PatchConstantPoolAccessInstruction.
Also, rename pos -> pc_offset
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#newcode1405
src/arm/assembler-arm.h:1405: // emitted before any use of db and dd to
ensure that constant pools
On 2015/05/20 14:32:10, rmcilroy wrote:
nit - update comment to include dq, dp and change "constant pool
should be
emitted" to "CheckConstPool() should be called"
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#newcode1492
src/arm/assembler-arm.h:1492: int EmitConstantPool() { return
constant_pool_builder_.Emit(this); }
On 2015/05/20 14:32:10, rmcilroy wrote:
EmitEmbeddedConstantPool (and add a DCHECK that the flag is true)
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#newcode1494
src/arm/assembler-arm.h:1494: bool ConstantPoolOverflow() const {
On 2015/05/20 14:32:10, rmcilroy wrote:
nit - how about ConstantPoolAccessIsInOverflow or something like that?
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/arm/full-codegen-arm.cc#newcode362
src/arm/full-codegen-arm.cc:362: masm()->EmitConstantPool();
On 2015/05/20 14:32:10, rmcilroy wrote:
Should we be emitting this here if we are also emitting it in
Assembler::GetCode?
Currently, the embedded constant pool is emitted immediately following
the instructions but prior to any other tables (e.g. safepoint,
back-edge). The intent was to keep the constant pool related logic
together and have the constant pool area immediately follow code in the
disassembly. I'll concede that there is no strict reason for this
ordering. (Note, however, that the constant pool builder is smart
enough to emit the table only on the first invocation).
Let me know if this is a significant issue and I will modify it to emit
at the end.
https://codereview.chromium.org/1131783003/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/arm/lithium-codegen-arm.cc#newcode387
src/arm/lithium-codegen-arm.cc:387: masm()->EmitConstantPool();
On 2015/05/20 14:32:10, rmcilroy wrote:
Same question as full-codegen
See earlier response.
https://codereview.chromium.org/1131783003/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/arm/macro-assembler-arm.cc#newcode984
src/arm/macro-assembler-arm.cc:984: void
MacroAssembler::LoadTargetConstantPoolPointerRegister(Register target) {
On 2015/05/20 14:32:10, rmcilroy wrote:
/s/LoadTargetConstantPoolPointerRegister/LoadConstantPoolPointerRegisterFromCodeTargetAddress
/s/target/code_target_address
Done.
https://codereview.chromium.org/1131783003/diff/1/src/arm64/deoptimizer-arm64.cc
File src/arm64/deoptimizer-arm64.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/arm64/deoptimizer-arm64.cc#newcode357
src/arm64/deoptimizer-arm64.cc:357: // No constant pool support.
On 2015/05/20 14:32:10, rmcilroy wrote:
No embedded constant .... (Arm64 uses inline constant pools)
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc
File src/assembler.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1698
src/assembler.cc:1698: (merged ? ConstantPoolEntry::REGULAR :
NextAccess(type));
On 2015/05/20 14:32:11, rmcilroy wrote:
Please add a DCHECK that merged entries are only in the regular
section here and
some kind of unittest would be nice too.
Done. Note that the check asserting that the offset is within reach
(during emit, prior to patching the instructions) provides a more
comprehensive sanity check for this.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1702
src/assembler.cc:1702: if (entry.sharing_ok() && !merged && access ==
ConstantPoolEntry::REGULAR) {
On 2015/05/20 14:32:10, rmcilroy wrote:
Did you do any profiling to show that this was more worthwhile than
doing
sharing across both REGULAR and OVERFLOW.
If left unbounded, the old O(n^2) search caused unacceptable compile
times that were actually noticeable to the user. I found this out the
hard way via octane. mandreel's global_init function has so many
constants eligible for sharing that it basically hung the benchmark.
Using the the regular area as the limit provides opportunities for
simplification vs using another arbitrary limit.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1751
src/assembler.cc:1751: }
On 2015/05/20 14:32:11, rmcilroy wrote:
Could you split the shared entry emitting out to a separate function,
it's a bit
confusing that it is lumped in here along with the begin/end setting
below.
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1769
src/assembler.cc:1769: if (it->merged_index_ < 0) {
On 2015/05/20 14:32:11, rmcilroy wrote:
nit - would probably be clearer to have a ConstantPoolEntry::is_merged
helper
function.
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1798
src/assembler.cc:1798:
info_[ConstantPoolEntry::DOUBLE].shared_entries.empty());
On 2015/05/20 14:32:11, rmcilroy wrote:
nit - add ConstantPoolBuilder::is_empty() helper function
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h
File src/assembler.h (right):
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode86
src/assembler.h:86: // Constant pool not supported on this architecture.
On 2015/05/20 14:32:11, rmcilroy wrote:
Embedded constant... (and below)
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1161
src/assembler.h:1161: merged_index_(sharing_ok ? -1 : -2),
On 2015/05/20 14:32:11, rmcilroy wrote:
Define and use constants for '-1' and '-2' to clarify their meaning
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1174
src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type ==
INTPTR) ? kPointerSize
On 2015/05/20 14:32:11, rmcilroy wrote:
I think all you need here is:
return (type == INTPTR) ? kPointerSize : kDoubleSize
My thinking was that the compiler could eliminate the condition all
together on 64-bit architectures.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1192
src/assembler.h:1192: class ConstantPoolBuilder BASE_EMBEDDED {
On 2015/05/20 14:32:11, rmcilroy wrote:
Thanks for factoring this out into common code. Could you please add
some
unittests which test this class and the various corner cases (e.g.,
overflowing
constant pools, checking the emitted layout etc.).
I will resolve the other comments here in the next iteration and tackle
the unittests in parallel with the 2nd review.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1194
src/assembler.h:1194: ConstantPoolBuilder(int ptr_reach, int
double_reach);
On 2015/05/20 14:32:11, rmcilroy wrote:
ptr_reach_bits and double_reach_bits
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1206
src/assembler.h:1206: inline Label* Position() { return &label_; }
On 2015/05/20 14:32:11, rmcilroy wrote:
/s/Position/EmittedPosition. Also some comments for each of the
public methods
would be nice.
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1214
src/assembler.h:1214: struct TypeInfo {
On 2015/05/20 14:32:11, rmcilroy wrote:
nit - TypeInfo seems a bit confusingly named. How about
PerTypeEntryDetails or
similar?
Done. PerTypeEntryInfo?
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1227
src/assembler.h:1227: Label label_;
On 2015/05/20 14:32:11, rmcilroy wrote:
/s/label_/emitted_label_ (and a comment describing what this is would
be nice).
Done.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1229
src/assembler.h:1229: };
On 2015/05/20 14:32:11, rmcilroy wrote:
two newlines here
Done.
https://codereview.chromium.org/1131783003/diff/1/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/compiler/code-generator.cc#newcode132
src/compiler/code-generator.cc:132: AssembleConstantPool();
On 2015/05/20 14:32:11, rmcilroy wrote:
Do we need this here too if we are doing it in GetCode? It seems like
there are
quite a few places we are emitting the constant pools when ideally I
would hope
there only needed to be a single place in GetCode().
See earlier response.
https://codereview.chromium.org/1131783003/diff/1/src/frames.cc
File src/frames.cc (left):
https://codereview.chromium.org/1131783003/diff/1/src/frames.cc#oldcode326
src/frames.cc:326: }
On 2015/05/20 14:32:11, rmcilroy wrote:
Why are you removing this?
By definition, the an exit frame's pc_address is above sp (see
ExitFrame::FillState). Thus, this check always fails.
This appears to be harmless for frames without the constant pool slot,
but causes real issues otherwise. IIRC, it leads to clobbering constant
pool register upon return in some cases.
https://codereview.chromium.org/1131783003/diff/1/src/frames.h
File src/frames.h (right):
https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode128
src/frames.h:128: static const int kConstantPoolOffset = kCPSlotSize ?
-1 * kPointerSize : 0;
On 2015/05/20 14:32:11, rmcilroy wrote:
nit - align '='
git cl format did this. Should I override?
https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode133
src/frames.h:133: static const int kLastObjectOffset = kContextOffset;
On 2015/05/20 14:32:11, rmcilroy wrote:
ditto
see above
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 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.
https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):
https://codereview.chromium.org/1131783003/diff/1/src/ia32/assembler-ia32.h#newcode1452
src/ia32/assembler-ia32.h:1452: void dp(uintptr_t data) { dd(data); }
On 2015/05/20 14:32:11, rmcilroy wrote:
I don't think you need dq and dp for the architectures which don't
support the
constant pool. Could you remove all of these to keep the CL smaller
and we can
add them back in the future if they become necessary.
The common constant pool builder code references these symbols. I could
ifdef that code, but prior experience has taught me that is frowned
upon. :)
https://codereview.chromium.org/1131783003/diff/1/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/ia32/deoptimizer-ia32.cc#newcode426
src/ia32/deoptimizer-ia32.cc:426: // No constant pool support.
On 2015/05/20 14:32:11, rmcilroy wrote:
embedded
Done.
https://codereview.chromium.org/1131783003/diff/1/src/mips/deoptimizer-mips.cc
File src/mips/deoptimizer-mips.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/mips/deoptimizer-mips.cc#newcode402
src/mips/deoptimizer-mips.cc:402: // No constant pool support.
On 2015/05/20 14:32:11, rmcilroy wrote:
embedded
Done.
https://codereview.chromium.org/1131783003/diff/1/src/mips64/deoptimizer-mips64.cc
File src/mips64/deoptimizer-mips64.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/mips64/deoptimizer-mips64.cc#newcode406
src/mips64/deoptimizer-mips64.cc:406: // No constant pool support.
On 2015/05/20 14:32:11, rmcilroy wrote:
embedded
Done.
https://codereview.chromium.org/1131783003/diff/1/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/objects.cc#newcode11843
src/objects.cc:11843: int constant_offset =
On 2015/05/20 14:32:12, rmcilroy wrote:
nit - constant_pool_offset
Done.
https://codereview.chromium.org/1131783003/diff/1/src/objects.cc#newcode11847
src/objects.cc:11847: int code_size = Min(safepoint_offset,
back_edge_offset);
On 2015/05/20 14:32:12, rmcilroy wrote:
Do we want the Min of constant_pool_offset as well as safepoint_offset
and
back_edge_offset?
Effectively yes -- that's result of the 'end' calculation below.
However, I need to preserve this value of 'code_size' for comparison
against constant_pool_offset below. Again, this code assumes that the
constant pool is emitted immediately following the instructions.
https://codereview.chromium.org/1131783003/diff/1/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/1131783003/diff/1/src/objects.h#newcode5300
src/objects.h:5300: static const int kCPSize =
FLAG_enable_embedded_constant_pool ? kIntSize : 0;
On 2015/05/20 14:32:12, rmcilroy wrote:
nit - kConstantPoolSize
Done.
https://codereview.chromium.org/1131783003/diff/1/src/ppc/assembler-ppc.cc
File src/ppc/assembler-ppc.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/ppc/assembler-ppc.cc#newcode208
src/ppc/assembler-ppc.cc:208: constant_pool_builder_(15, 15), // 15-bit
unsigned reach
On 2015/05/20 14:32:12, rmcilroy wrote:
same comments as Arm
Done.
https://codereview.chromium.org/1131783003/diff/1/src/x87/deoptimizer-x87.cc
File src/x87/deoptimizer-x87.cc (right):
https://codereview.chromium.org/1131783003/diff/1/src/x87/deoptimizer-x87.cc#newcode465
src/x87/deoptimizer-x87.cc:465: // No constant pool support.
On 2015/05/20 14:32:12, rmcilroy wrote:
embedded
Done.
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.
