This is looking great Matthew, this is exaclty the direction I was hoping
for -
thanks!
I have a load of comments but most are just nits. I've not looked closely
in the
PPC port, I'm assuming you know what your doing here ;). In general, I would
suggest the same comments in PPC as I made in Arm though.
Also, could you please add "BUG=chromium:478811" in the description and
update
the title to "Add support for Embedded Constant Pools for PPC and Arm" and
mention in the description that you are removing support for OOL constant
pools.
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),
make the '12' and '10' constants defined in constants-arm.h
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#newcode1083
src/arm/assembler-arm.cc:1083: // An extended constant pool load.
update comment.
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.cc#newcode3592
src/arm/assembler-arm.cc:3592: CheckBuffer();
Please add the num_pending_xx_bit_constants_ == 0 DCHECKs here too.
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,
I think this would be clearer as something like
PatchConstantPoolAccessInstruction.
Also, rename pos -> pc_offset
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
nit - update comment to include dq, dp and change "constant pool should
be emitted" to "CheckConstPool() should be called"
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); }
EmitEmbeddedConstantPool (and add a DCHECK that the flag is true)
https://codereview.chromium.org/1131783003/diff/1/src/arm/assembler-arm.h#newcode1494
src/arm/assembler-arm.h:1494: bool ConstantPoolOverflow() const {
nit - how about ConstantPoolAccessIsInOverflow or something like that?
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();
Should we be emitting this here if we are also emitting it in
Assembler::GetCode?
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();
Same question as full-codegen
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) {
/s/LoadTargetConstantPoolPointerRegister/LoadConstantPoolPointerRegisterFromCodeTargetAddress
/s/target/code_target_address
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.
No embedded constant .... (Arm64 uses inline constant pools)
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));
Please add a DCHECK that merged entries are only in the regular section
here and some kind of unittest would be nice too.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1702
src/assembler.cc:1702: if (entry.sharing_ok() && !merged && access ==
ConstantPoolEntry::REGULAR) {
Did you do any profiling to show that this was more worthwhile than
doing sharing across both REGULAR and OVERFLOW.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1751
src/assembler.cc:1751: }
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.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1769
src/assembler.cc:1769: if (it->merged_index_ < 0) {
nit - would probably be clearer to have a ConstantPoolEntry::is_merged
helper function.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.cc#newcode1798
src/assembler.cc:1798:
info_[ConstantPoolEntry::DOUBLE].shared_entries.empty());
nit - add ConstantPoolBuilder::is_empty() helper function
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.
Embedded constant... (and below)
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1161
src/assembler.h:1161: merged_index_(sharing_ok ? -1 : -2),
Define and use constants for '-1' and '-2' to clarify their meaning
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1174
src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type ==
INTPTR) ? kPointerSize
I think all you need here is:
return (type == INTPTR) ? kPointerSize : kDoubleSize
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1192
src/assembler.h:1192: class ConstantPoolBuilder BASE_EMBEDDED {
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.).
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1194
src/assembler.h:1194: ConstantPoolBuilder(int ptr_reach, int
double_reach);
ptr_reach_bits and double_reach_bits
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1206
src/assembler.h:1206: inline Label* Position() { return &label_; }
/s/Position/EmittedPosition. Also some comments for each of the public
methods would be nice.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1214
src/assembler.h:1214: struct TypeInfo {
nit - TypeInfo seems a bit confusingly named. How about
PerTypeEntryDetails or similar?
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1227
src/assembler.h:1227: Label label_;
/s/label_/emitted_label_ (and a comment describing what this is would be
nice).
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1229
src/assembler.h:1229: };
two newlines here
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();
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().
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: }
Why are you removing this?
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;
nit - align '='
https://codereview.chromium.org/1131783003/diff/1/src/frames.h#newcode133
src/frames.h:133: static const int kLastObjectOffset = kContextOffset;
ditto
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;
why are these changes to add OBJECT_SLOT to the mark compact collector
required? Can they be made in a different CL?
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); }
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.
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.
embedded
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.
embedded
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.
embedded
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 =
nit - constant_pool_offset
https://codereview.chromium.org/1131783003/diff/1/src/objects.cc#newcode11847
src/objects.cc:11847: int code_size = Min(safepoint_offset,
back_edge_offset);
Do we want the Min of constant_pool_offset as well as safepoint_offset
and back_edge_offset?
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;
nit - kConstantPoolSize
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
same comments as Arm
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.
embedded
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.
