I'll do a last pass once you have the tests.
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 22:28:21, mtbrandyberry wrote:
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.
I think this would be better right at the end just before the relocinfo
(which is what describes the constant pool anyway). I don't think this
should make the disassembly any more complex, but if it does let me know
and I'll re-evaluate.
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 22:28:22, mtbrandyberry wrote:
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.
Yes, but it is also good to get DCHECKs early where possible to make
debuggging easier. Thanks for adding the DCHECK.
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 22:28:22, mtbrandyberry wrote:
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.
Right, thanks for the explination. I had issues with mandreel's
global_init function as well. In this case I'm fine with this.
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#newcode1174
src/assembler.h:1174: return ((kPointerSize == kDoubleSize || type ==
INTPTR) ? kPointerSize
On 2015/05/20 22:28:22, mtbrandyberry wrote:
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.
If the compiler is smart enough to do that, then it is also probably
smart enough to do constant propigation and work out kPointerSize and
kDoubleSize are the same and also eliminate the condition. In anycase, I
don't think this microoptimisation is worth the reduced readability
unless you have specific numbers to back up it being worthwhile.
https://codereview.chromium.org/1131783003/diff/1/src/assembler.h#newcode1214
src/assembler.h:1214: struct TypeInfo {
On 2015/05/20 22:28:22, mtbrandyberry wrote:
On 2015/05/20 14:32:11, rmcilroy wrote:
> nit - TypeInfo seems a bit confusingly named. How about
PerTypeEntryDetails or
> similar?
Done. PerTypeEntryInfo?
SGTM
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 22:28:22, mtbrandyberry wrote:
On 2015/05/20 14:32:11, rmcilroy wrote:
> nit - align '='
git cl format did this. Should I override?
Ahh right, in that case could you touch all the constants in this block
so git cl format lines makes them all two-space instead of lined up.
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 22:28:22, mtbrandyberry wrote:
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. :)
Right enough. In that case this is fine.
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.
