Remove relocation lock.

Remove relocation lock.

Freeze HValue hash codes that are based on object addresses.

R=svenpanne@chromium.org
BUG=

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

[Sven Panne, 2013-04-12 11:30:53]

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3298: if (!has_double_value_) {
What about has_int32_value_?

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3354: // So has_double_value_ governs how to
interpret the union.
This is a bit obscure. Wouldn't it be better if we had some enum (perhaps stored
in a bitfield tag for space reasons) with 3 values
HAS_{INT32,DOUBLE,OBJECT}_VALUE and include the double in the union?

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4800: intptr_t hash_code_;
Renaming this to something like 'unique_id_' makes it a bit clearer that it is
actually a bit more than a hash value.

https://codereview.chromium.org/14040006/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14040006/diff/1/src/hydrogen.cc#newcode1704
src/hydrogen.cc:1704: for (HInstruction* instr = blocks()->at(i)->first();
Nano-optimization: Perhaps name the 'blocks()->at(i)' subexpression, it might be
hard for the compiler to see that this can be hoisted.

[Sven Panne, 2013-04-12 13:47:02]

LGTM with a nit

https://codereview.chromium.org/14040006/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14040006/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3208: CHECK_NE(NULL, raw_address_);
ASSERT_NE

[Yang, 2013-04-15 13:55:31]

Addressed comments. I found that more HValues had to be adapted. Please take a
look at those changes as well.

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3298: if (!has_double_value_) {
On 2013/04/12 11:30:53, Sven Panne wrote:
> What about has_int32_value_?

Done.

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3354: // So has_double_value_ governs how to
interpret the union.
On 2013/04/12 11:30:53, Sven Panne wrote:
> This is a bit obscure. Wouldn't it be better if we had some enum (perhaps
stored
> in a bitfield tag for space reasons) with 3 values
> HAS_{INT32,DOUBLE,OBJECT}_VALUE and include the double in the union?

Skipping this comment as discussed offline.

https://codereview.chromium.org/14040006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4800: intptr_t hash_code_;
On 2013/04/12 11:30:53, Sven Panne wrote:
> Renaming this to something like 'unique_id_' makes it a bit clearer that it is
> actually a bit more than a hash value.

Done.

https://codereview.chromium.org/14040006/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14040006/diff/1/src/hydrogen.cc#newcode1704
src/hydrogen.cc:1704: for (HInstruction* instr = blocks()->at(i)->first();
On 2013/04/12 11:30:53, Sven Panne wrote:
> Nano-optimization: Perhaps name the 'blocks()->at(i)' subexpression, it might
be
> hard for the compiler to see that this can be hoisted.

Skipped as discussed offline.

[Sven Panne, 2013-04-16 06:43:44]

A general remark: Introducing a separate simple class for the Unique IDs might
be a good idea. Using a mix of intptr_t and Address is confusing, and Address
always has the connotation "I can be dereferenced", which is wrong. The only
operations we need is construction from a Handle(?), equality and retrieving a
hash value:

template<class T>
class UniqueValueId {
   explicit UniqueValueId(Handle<T> h);
   bool operator==(const UniqueValueId<T>& other) const;
   intptr_t Hashcode() const;
};

(Probably we should define !=, too)

https://codereview.chromium.org/14040006/diff/11001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14040006/diff/11001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2541: // Raw addresses may have already been
collected.
I don't understand this comment here and elsewhere.

[Yang, 2013-04-16 09:34:09]

On 2013/04/16 06:43:44, Sven Panne wrote:
> A general remark: Introducing a separate simple class for the Unique IDs might
> be a good idea. Using a mix of intptr_t and Address is confusing, and Address
> always has the connotation "I can be dereferenced", which is wrong. The only
> operations we need is construction from a Handle(?), equality and retrieving a
> hash value:
> 
> template<class T>
> class UniqueValueId {
>    explicit UniqueValueId(Handle<T> h);
>    bool operator==(const UniqueValueId<T>& other) const;
>    intptr_t Hashcode() const;
> };
> 
> (Probably we should define !=, too)
> 
>
https://codereview.chromium.org/14040006/diff/11001/src/hydrogen-instructions.cc
> File src/hydrogen-instructions.cc (right):
> 
>
https://codereview.chromium.org/14040006/diff/11001/src/hydrogen-instructions...
> src/hydrogen-instructions.cc:2541: // Raw addresses may have already been
> collected.
> I don't understand this comment here and elsewhere.

Done. Please take another look.

[Sven Panne, 2013-04-16 09:46:42]

Almost there...

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions...
src/hydrogen-instructions.h:396: bool IsImmovableObject(Object* object) const {
Slightly obscure, and we don't need this (see below).

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions...
src/hydrogen-instructions.h:1100: // Some instructions' hash code is dependent
on object addresses and are not
Adapt comment: We are talking about unique IDs here, not only hash codes.

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3266:
ASSERT(!unique_id_.IsImmovableObject(heap->minus_zero_value()));
Just construct a UniqueValueId for the RHS and use == or != here and below, it
is compiled away anyway.

[Yang, 2013-04-16 09:59:06]

Addressed comments.

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions...
src/hydrogen-instructions.h:396: bool IsImmovableObject(Object* object) const {
On 2013/04/16 09:46:42, Sven Panne wrote:
> Slightly obscure, and we don't need this (see below).

Done.

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions...
src/hydrogen-instructions.h:1100: // Some instructions' hash code is dependent
on object addresses and are not
On 2013/04/16 09:46:42, Sven Panne wrote:
> Adapt comment: We are talking about unique IDs here, not only hash codes.

Done.

https://codereview.chromium.org/14040006/diff/18001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3266:
ASSERT(!unique_id_.IsImmovableObject(heap->minus_zero_value()));
On 2013/04/16 09:46:42, Sven Panne wrote:
> Just construct a UniqueValueId for the RHS and use == or != here and below, it
> is compiled away anyway.

Done.

[Sven Panne, 2013-04-16 10:35:26]

LGTM if the nit is addressed.

https://codereview.chromium.org/14040006/diff/20010/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14040006/diff/20010/src/hydrogen-instructions...
src/hydrogen-instructions.h:379: bool IsUninitialized() const { return
raw_address_ == NULL; }
Almost all uses are negated, so let's negate the test itself and have an
IsInitialized() predicate, see
http://www.refactoring.com/catalog/removeDoubleNegative.html.

[Yang, 2013-04-16 11:00:15]

Committed patchset #7 manually as r14273 (presubmit successful).