Looks like a nice cleanup overall, couple of comments.
Note - the Arm backend is very close to switching over to the OOL constant
pool,
and we want to switch Arm64 to this soon after. I'm not sure if you have
any
more plans for the Arm64 constant pool, but if so it may make more sense to
wait
for the OOL Arm64 constant pool before putting more efforts into this (and
if
you can help with the OOL arm64 implementation that would be great too).
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc
File src/arm64/assembler-arm64.cc (right):
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode339
src/arm64/assembler-arm64.cc:339: // will be accessing the last entry.
nit - this comment was a bit unclear to me first (I thought you were
explicitly shuffling to ensure the offsets stayed in range). How about:
Entries are not necessarily emitted in the order they are accessed, so
in the worst case....
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode376
src/arm64/assembler-arm64.cc:376:
ASSERT(assm_->is_const_pool_blocked());
This is a bit counter intuitive - maybe have the BlockConstantPool
object in this function instead of the caller, and:
ASSERT(!assm->is_const_pool_blocked());
BlockPoolsScope block_pools(this);
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode473
src/arm64/assembler-arm64.cc:473: typedef std::multimap<uint64_t,
int>::const_iterator shared_entries_it;
Use Type style name for shared_entries_it (e.g., CapitalCase like other
C++ types in V8).
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode486
src/arm64/assembler-arm64.cc:486: shared_entries_.erase(data);
nit - could you leave the entry here and just do shared_entries_.clear()
after the loop? That might be slightly more efficient.
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode2676
src/arm64/assembler-arm64.cc:2676: if (constpool_.EntryCount() >
kApproximatePoolEntryCount) {
Could we just do both this check and the check for
kApproximateDistToConstPool in RecordEntry() and set
next_const_pool_check_ = pc_offset() + kInstructionSize in that function
if either exceed the maximum?
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode2753
src/arm64/assembler-arm64.cc:2753: int size =
constpool_.WorstCaseSize();
s/size/worst_case_size
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode2756
src/arm64/assembler-arm64.cc:2756: // Buffer checks happen after an emit
hence the 2 * kInstructionSize.
I'm not sure what the buffer checks you mention here are (and they don't
seem to have been included in the check before), could you clarify
please?
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode2774
src/arm64/assembler-arm64.cc:2774: }
Add an assert that the size of code generated was less than
worst_case_size
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.h
File src/arm64/assembler-arm64.h (right):
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.h#newcode768
src/arm64/assembler-arm64.h:768: void Emit(bool require_jump);
comments on Emit and Clear please
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.h#newcode2103
src/arm64/assembler-arm64.h:2103: // * code generation is finished
Is it no longer the case that they are emitted in dead code locations
(or was the previous comment wrong)? dead code locations are not
necessarily only when code generation is finished.
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.h#newcode2122
src/arm64/assembler-arm64.h:2122: static const int
kApproximateDistToConstPool = 64 * KB;
How about kApproxMaxDistToConstPool?
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.h#newcode2126
src/arm64/assembler-arm64.h:2126: static const int
kApproximatePoolEntryCount = 512;
kApproxMaxPoolEntryCount?
https://codereview.chromium.org/338523005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
