I only plan one further patch to enable more instructions to use the literal
pool for 64-bit immediate so more entries can be shared but that's it.
Of course, We'll help landing OOL pool for arm64.
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.
On 2014/06/17 13:09:56, rmcilroy wrote:
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....
Done.
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());
On 2014/06/17 13:09:57, rmcilroy wrote:
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);
Done.
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;
On 2014/06/17 13:09:57, rmcilroy wrote:
Use Type style name for shared_entries_it (e.g., CapitalCase like
other C++
types in V8).
Done.
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.cc#newcode486
src/arm64/assembler-arm64.cc:486: shared_entries_.erase(data);
Loop updated to iterate through literals and then clear the map.
On 2014/06/17 13:09:57, rmcilroy wrote:
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) {
moving the this code in RecordEntry is a good idea. CheckConstPool still
need to
check dist and count though.
On 2014/06/17 13:09:57, rmcilroy wrote:
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();
On 2014/06/17 13:09:57, rmcilroy wrote:
s/size/worst_case_size
Done.
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.
My comment is clumsy and inaccurate. What I meant is that every
instruction emit will check if the free buffer space is bigger than
kGap but this check happens after the instruction emit (this was
inherited from ARM).
So if you want to ensure the buffer will not grow when you are about to
emit n
instructions you need to ensure you have n + kGap + 1 instructions of
space. I
put 2 because I also changed the comparison below to < from <= which
actually
makes it more confusing, I'll revert.
On 2014/06/17 13:09:57, rmcilroy wrote:
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: }
On 2014/06/17 13:09:57, rmcilroy wrote:
Add an assert that the size of code generated was less than
worst_case_size
Done.
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);
On 2014/06/17 13:09:57, rmcilroy wrote:
comments on Emit and Clear please
Done.
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.h#newcode2103
src/arm64/assembler-arm64.h:2103: // * code generation is finished
No we no longer emit in dead code location. On ARM because of the small
load
literal range it was a nice optimisation but on a64 it would only
contribute to
ICache pollution.
On 2014/06/17 13:09:57, rmcilroy wrote:
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;
On 2014/06/17 13:09:57, rmcilroy wrote:
How about kApproxMaxDistToConstPool?
Done.
https://codereview.chromium.org/338523005/diff/1/src/arm64/assembler-arm64.h#newcode2126
src/arm64/assembler-arm64.h:2126: static const int
kApproximatePoolEntryCount = 512;
On 2014/06/17 13:09:57, rmcilroy wrote:
kApproxMaxPoolEntryCount?
Done.
https://codereview.chromium.org/338523005/
--
--
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.
