ARM64: updated literal pool implementation.

ARM64: updated literal pool implementation.

Currently the literal pool implemetation is inherited from the arm 32-bit port
and it shares the same limitations: 4k of range and 1000 entries max. In arm64
the load literal has a 1MB range giving us more flexibility.

Immutable entries are now shared.

BUG=
R=rmcilroy@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21924

[ulan, 2014-06-14 08:00:48]

Adding Ross since he is working on constant pool too.

[rmcilroy, 2014-06-17 13:09:58]

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...
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...
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...
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...
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...
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...
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...
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...
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#...
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#...
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#...
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#...
src/arm64/assembler-arm64.h:2126: static const int kApproximatePoolEntryCount =
512;
kApproxMaxPoolEntryCount?

[Rodolph Perfetta (ARM), 2014-06-18 16:53:23]

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...
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...
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...
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...
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...
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...
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...
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...
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#...
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#...
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#...
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#...
src/arm64/assembler-arm64.h:2126: static const int kApproximatePoolEntryCount =
512;
On 2014/06/17 13:09:57, rmcilroy wrote:
> kApproxMaxPoolEntryCount?

Done.

[rmcilroy, 2014-06-19 09:58:16]

lgtm, thanks!

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#...
src/arm64/assembler-arm64.h:2103: //  * code generation is finished
On 2014/06/18 16:53:23, Rodolph Perfetta (ARM) wrote:
> 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.

Interesting. Sounds good in that case.

[Rodolph Perfetta (ARM), 2014-06-23 09:30:58]

Committed patchset #2 manually as r21924 (presubmit successful).