Fixed an issue with HConstant::InNewSpace() for parallel compilation. (issue 14159028)

[mvst...@chromium.org]

Reviewers: Michael Starzinger,

Message:
Here are the changes we discussed. Care is taken to avoid dereferencing the
handle if we are running on the optimizer thread.

Description:
Fixed an issue with HConstant::InNewSpace() for parallel compilation.

Better to save the information about a constant handle being in new space at
hydrogen time.

BUG=

Please review this at https://codereview.chromium.org/14159028/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/hydrogen-instructions.h
M src/hydrogen-instructions.cc
M src/ia32/lithium-ia32.cc
M src/x64/lithium-x64.cc


Index: src/hydrogen-instructions.cc
diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc
index
aadf87572df411eb5d8c9c9aeb1ce8b985010aea..d47c6efc67d2ba8f3defc21776c6153d454cdde0
100644
--- a/src/hydrogen-instructions.cc
+++ b/src/hydrogen-instructions.cc
@@ -2073,6 +2073,7 @@ HConstant::HConstant(Handle<Object> handle,
Representation r)
type_from_value_ = HType::TypeFromValue(handle_);
is_internalized_string_ = handle_->IsInternalizedString();
}
+
if (r.IsNone()) {
if (has_int32_value_) {
r = Representation::Integer32();
@@ -2142,34 +2143,56 @@ void HConstant::Initialize(Representation r) {
if (representation().IsInteger32()) {
ClearGVNFlag(kDependsOnOsrEntries);
}
+
+ // TODO(mvstanton): pass isolate as a parameter
+ Isolate* isolate = Isolate::Current();
+ if (handle_.is_null()) {
+ guaranteed_in_old_space_ = true;
+ } else if (!isolate->optimizing_compiler_thread()->IsOptimizerThread()) {
+ ALLOW_HANDLE_DEREF(isolate, "using raw address");
+ guaranteed_in_old_space_ = !(isolate->heap()->InNewSpace(*handle_));
+ } else {
+ guaranteed_in_old_space_ = false;
+ }
}


HConstant* HConstant::CopyToRepresentation(Representation r, Zone* zone)
const {
if (r.IsInteger32() && !has_int32_value_) return NULL;
if (r.IsDouble() && !has_double_value_) return NULL;
- if (has_int32_value_) return new(zone) HConstant(int32_value_, r,
handle_);
- if (has_double_value_) return new(zone) HConstant(double_value_, r,
handle_);
- ASSERT(!handle_.is_null());
- return new(zone) HConstant(handle_,
- unique_id_,
- r,
- type_from_value_,
- is_internalized_string_,
- boolean_value_);
+ HConstant* result;
+ if (has_int32_value_) {
+ result = new(zone) HConstant(int32_value_, r, handle_);
+ } else if (has_double_value_) {
+ result = new(zone) HConstant(double_value_, r, handle_);
+ } else {
+ ASSERT(!handle_.is_null());
+ result = new(zone) HConstant(handle_,
+ unique_id_,
+ r,
+ type_from_value_,
+ is_internalized_string_,
+ boolean_value_);
+ }
+ result->set_guaranteed_in_old_space(guaranteed_in_old_space_);
+ return result;
}


HConstant* HConstant::CopyToTruncatedInt32(Zone* zone) const {
+ HConstant* result;
if (has_int32_value_) {
- return new(zone) HConstant(
+ result = new(zone) HConstant(
int32_value_, Representation::Integer32(), handle_);
- }
- if (has_double_value_) {
- return new(zone) HConstant(
+ } else if (has_double_value_) {
+ result = new(zone) HConstant(
DoubleToInt32(double_value_), Representation::Integer32(),
handle_);
+ } else {
+ return NULL;
}
- return NULL;
+
+ result->set_guaranteed_in_old_space(guaranteed_in_old_space_);
+ return result;
}


Index: src/hydrogen-instructions.h
diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index
3090f46f1f050bc0b810a9d1a4a90bb4e8026f76..7722e1d9edb59627a98d99acc23c49452305c7ba
100644
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -3199,7 +3199,9 @@ class HConstant: public HTemplateInstruction<0> {

Handle<Object> handle() {
if (handle_.is_null()) {
- handle_ = FACTORY->NewNumber(double_value_, pretenure());
+ // The calculation for not_in_new_space_ must be revisited
+ // if the handle isn't created TENURED here.
+ handle_ = FACTORY->NewNumber(double_value_, TENURED);
}
ALLOW_HANDLE_DEREF(Isolate::Current(), "smi check");
ASSERT(has_int32_value_ || !handle_->IsSmi());
@@ -3213,16 +3215,7 @@ class HConstant: public HTemplateInstruction<0> {
std::isnan(double_value_));
}

- bool InNewSpace() const {
- if (!handle_.is_null()) {
- ALLOW_HANDLE_DEREF(isolate(), "using raw address");
- return isolate()->heap()->InNewSpace(*handle_);
- }
- // If the handle wasn't created yet, then we have a number.
- // If the handle is created it'll be tenured in old space.
- ASSERT(pretenure() == TENURED);
- return false;
- }
+ bool GuaranteedInOldSpace() const { return guaranteed_in_old_space_; }

bool ImmortalImmovable() const {
if (has_int32_value_) {
@@ -3355,12 +3348,14 @@ class HConstant: public HTemplateInstruction<0> {

virtual bool IsDeletable() const { return true; }

+ void set_guaranteed_in_old_space(bool value) {
+ guaranteed_in_old_space_ = value;
+ }
+
// If this is a numerical constant, handle_ either points to to the
// HeapObject the constant originated from or is null. If the
// constant is non-numeric, handle_ always points to a valid
// constant HeapObject.
- static PretenureFlag pretenure() { return TENURED; }
-
Handle<Object> handle_;
UniqueValueId unique_id_;

@@ -3373,6 +3368,7 @@ class HConstant: public HTemplateInstruction<0> {
bool has_double_value_ : 1;
bool is_internalized_string_ : 1; // TODO(yangguo): make this part of
HType.
bool boolean_value_ : 1;
+ bool guaranteed_in_old_space_ : 1;
int32_t int32_value_;
double double_value_;
HType type_from_value_;
Index: src/ia32/lithium-ia32.cc
diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc
index
7ae1f01cd43203db4802a1578c5b2bbdc6b3fd5c..7a1c4cea6cee05515e5d4a6ea4b44bf29a4af3c1
100644
--- a/src/ia32/lithium-ia32.cc
+++ b/src/ia32/lithium-ia32.cc
@@ -2423,7 +2423,7 @@ LInstruction*
LChunkBuilder::DoStoreNamedField(HStoreNamedField* instr) {
}

bool can_be_constant = instr->value()->IsConstant() &&
- !HConstant::cast(instr->value())->InNewSpace() &&
+ HConstant::cast(instr->value())->GuaranteedInOldSpace() &&
!(FLAG_track_double_fields &&
instr->field_representation().IsDouble());

LOperand* val;
Index: src/x64/lithium-x64.cc
diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc
index
4c5819076b913cbe0b3343b1aadba913bee6196e..dc74855e79d9bb302e8d4acc33bd8d083e9343a1
100644
--- a/src/x64/lithium-x64.cc
+++ b/src/x64/lithium-x64.cc
@@ -2246,7 +2246,7 @@ LInstruction*
LChunkBuilder::DoStoreNamedField(HStoreNamedField* instr) {
}

bool can_be_constant = instr->value()->IsConstant() &&
- !HConstant::cast(instr->value())->InNewSpace() &&
+ HConstant::cast(instr->value())->GuaranteedInOldSpace() &&
!(FLAG_track_double_fields &&
instr->field_representation().IsDouble());

LOperand* val;

[mstar...@chromium.org]

There are two reasons I am not particular fond of this change. The first is
that
it tightly couples HConstant with the parallel compiler thread and whether
we
are on of off that thread, as the changes in optimizing-compiler-thread.h
show.
The second is that HConstant::Initialize sometimes uses an estimate for the
predicate and sometimes it exactly queries the current object location, it
is
not obvious which is used when.

I have an alternative counter-proposal that always uses the exact object
location when an HConstant for a handle is constructed. When an existing
HConstant is copied, the original value is threaded through. And when an
heap
number does not yet exists, we can guarantee is to not be in new-space. See
https://codereview.chromium.org/13977019/ for my proposal.

https://codereview.chromium.org/14159028/