On 2015/05/22 12:21:22, rmcilroy wrote:
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.
I was out of the office for a few days. Picking this back up.
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.
