Fix constructors for HLoadNamedField.

Also try to determine an appropriate HType for tracked fields,
instead of (incorrectly) using HType::NonPrimitive() everywhere.

R=svenpanne@chromium.org

Review URL: https://codereview.chromium.org/269353003

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21186 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/hydrogen-check-elimination.cc b/src/hydrogen-check-elimination.cc
index bf549c5..701d965 100644 (file)
--- a/src/hydrogen-check-elimination.cc
+++ b/src/hydrogen-check-elimination.cc
@@ -340,8 +340,10 @@ class HCheckTable : public ZoneObject {
     // Reduce a load of the map field when it is known to be a constant.
     if (!instr->access().IsMap()) {
       // Check if we introduce field maps here.
-      if (instr->maps()->size() != 0) {
-        Insert(instr, instr, instr->maps());
+      MapSet maps = instr->maps();
+      if (maps != NULL) {
+        ASSERT_NE(0, maps->size());
+        Insert(instr, NULL, maps);
       }
       return;
     }

diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc
index d13b7ee..f5c5c32 100644 (file)
--- a/src/hydrogen-instructions.cc
+++ b/src/hydrogen-instructions.cc
@@ -3418,7 +3418,7 @@ void HLoadNamedField::PrintDataTo(StringStream* stream) {
   object()->PrintNameTo(stream);
   access_.PrintTo(stream);
 
-  if (maps()->size() != 0) {
+  if (maps() != NULL) {
     stream->Add(" [%p", *maps()->at(0).handle());
     for (int i = 1; i < maps()->size(); ++i) {
       stream->Add(",%p", *maps()->at(i).handle());

diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index 21e9551..1cdca4c 100644 (file)
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -6212,28 +6212,10 @@ class HObjectAccess V8_FINAL {
 
 class HLoadNamedField V8_FINAL : public HTemplateInstruction<2> {
  public:
-  static HLoadNamedField* New(Zone* zone, HValue* context,
-                              HValue* object, HValue* dependency,
-                              HObjectAccess access) {
-    return new(zone) HLoadNamedField(
-        object, dependency, access, new(zone) UniqueSet<Map>());
-  }
-  static HLoadNamedField* New(Zone* zone, HValue* context,
-                              HValue* object, HValue* dependency,
-                              HObjectAccess access, SmallMapList* map_list,
-                              CompilationInfo* info) {
-    UniqueSet<Map>* maps = new(zone) UniqueSet<Map>(map_list->length(), zone);
-    for (int i = 0; i < map_list->length(); ++i) {
-      Handle<Map> map = map_list->at(i);
-      maps->Add(Unique<Map>::CreateImmovable(map), zone);
-      // TODO(bmeurer): Get rid of this shit!
-      if (map->CanTransition()) {
-        Map::AddDependentCompilationInfo(
-            map, DependentCode::kPrototypeCheckGroup, info);
-      }
-    }
-    return new(zone) HLoadNamedField(object, dependency, access, maps);
-  }
+  DECLARE_INSTRUCTION_FACTORY_P3(HLoadNamedField, HValue*,
+                                 HValue*, HObjectAccess);
+  DECLARE_INSTRUCTION_FACTORY_P5(HLoadNamedField, HValue*, HValue*,
+                                 HObjectAccess, const UniqueSet<Map>*, HType);
 
   HValue* object() { return OperandAt(0); }
   HValue* dependency() {
@@ -6262,23 +6244,36 @@ class HLoadNamedField V8_FINAL : public HTemplateInstruction<2> {
   virtual Range* InferRange(Zone* zone) V8_OVERRIDE;
   virtual void PrintDataTo(StringStream* stream) V8_OVERRIDE;
 
+  bool CanBeReplacedWith(HValue* other) const {
+    if (!type().Equals(other->type())) return false;
+    if (!representation().Equals(other->representation())) return false;
+    if (!other->IsLoadNamedField()) return true;
+    HLoadNamedField* that = HLoadNamedField::cast(other);
+    if (this->maps_ == that->maps_) return true;
+    if (this->maps_ == NULL || that->maps_ == NULL) return false;
+    return this->maps_->IsSubset(that->maps_);
+  }
+
   DECLARE_CONCRETE_INSTRUCTION(LoadNamedField)
 
  protected:
   virtual bool DataEquals(HValue* other) V8_OVERRIDE {
-    HLoadNamedField* b = HLoadNamedField::cast(other);
-    return access_.Equals(b->access_) && this->maps()->Equals(b->maps());
+    HLoadNamedField* that = HLoadNamedField::cast(other);
+    if (!this->access_.Equals(that->access_)) return false;
+    if (this->maps_ == that->maps_) return true;
+    return (this->maps_ != NULL &&
+            that->maps_ != NULL &&
+            this->maps_->Equals(that->maps_));
   }
 
  private:
   HLoadNamedField(HValue* object,
                   HValue* dependency,
-                  HObjectAccess access,
-                  const UniqueSet<Map>* maps)
-      : access_(access), maps_(maps) {
-    ASSERT(object != NULL);
+                  HObjectAccess access)
+      : access_(access), maps_(NULL) {
+    ASSERT_NOT_NULL(object);
     SetOperandAt(0, object);
-    SetOperandAt(1, dependency != NULL ? dependency : object);
+    SetOperandAt(1, dependency ? dependency : object);
 
     Representation representation = access.representation();
     if (representation.IsInteger8() ||
@@ -6298,6 +6293,8 @@ class HLoadNamedField V8_FINAL : public HTemplateInstruction<2> {
                representation.IsInteger32()) {
       set_representation(representation);
     } else if (representation.IsHeapObject()) {
+      // TODO(bmeurer): This is probably broken. What we actually want to to
+      // instead is set_representation(Representation::HeapObject()).
       set_type(HType::NonPrimitive());
       set_representation(Representation::Tagged());
     } else {
@@ -6306,6 +6303,28 @@ class HLoadNamedField V8_FINAL : public HTemplateInstruction<2> {
     access.SetGVNFlags(this, LOAD);
   }
 
+  HLoadNamedField(HValue* object,
+                  HValue* dependency,
+                  HObjectAccess access,
+                  const UniqueSet<Map>* maps,
+                  HType type)
+      : HTemplateInstruction<2>(type), access_(access), maps_(maps) {
+    ASSERT_NOT_NULL(maps);
+    ASSERT_NE(0, maps->size());
+
+    ASSERT_NOT_NULL(object);
+    SetOperandAt(0, object);
+    SetOperandAt(1, dependency ? dependency : object);
+
+    ASSERT(access.representation().IsHeapObject());
+    // TODO(bmeurer): This is probably broken. What we actually want to to
+    // instead is set_representation(Representation::HeapObject()).
+    if (!type.IsHeapObject()) set_type(HType::NonPrimitive());
+    set_representation(Representation::Tagged());
+
+    access.SetGVNFlags(this, LOAD);
+  }
+
   virtual bool IsDeletable() const V8_OVERRIDE { return true; }
 
   HObjectAccess access_;

diff --git a/src/hydrogen-load-elimination.cc b/src/hydrogen-load-elimination.cc
index 836bd2d..1198d2b 100644 (file)
--- a/src/hydrogen-load-elimination.cc
+++ b/src/hydrogen-load-elimination.cc
@@ -53,12 +53,7 @@ class HLoadEliminationTable : public ZoneObject {
                FieldOf(l->access()),
                l->object()->ActualValue()->id()));
         HValue* result = load(l);
-        if (result != instr &&
-            result->type().Equals(instr->type()) &&
-            result->representation().Equals(instr->representation()) &&
-            (!result->IsLoadNamedField() ||
-             HLoadNamedField::cast(instr)->maps()->IsSubset(
-                 HLoadNamedField::cast(result)->maps()))) {
+        if (result != instr && l->CanBeReplacedWith(result)) {
           // The load can be replaced with a previous load or a value.
           TRACE(("  replace L%d -> v%d\n", instr->id(), result->id()));
           instr->DeleteAndReplaceWith(result);

diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 627a21d..ee9f8e4 100644 (file)
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -5348,8 +5348,24 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadNamedField(
     // Load the double value from it.
     access = HObjectAccess::ForHeapNumberValue();
   }
+
+  SmallMapList* map_list = info->field_maps();
+  if (map_list->length() == 0) {
+    return New<HLoadNamedField>(checked_object, checked_object, access);
+  }
+
+  UniqueSet<Map>* maps = new(zone()) UniqueSet<Map>(map_list->length(), zone());
+  for (int i = 0; i < map_list->length(); ++i) {
+    Handle<Map> map = map_list->at(i);
+    maps->Add(Unique<Map>::CreateImmovable(map), zone());
+    // TODO(bmeurer): Get rid of this shit!
+    if (map->CanTransition()) {
+      Map::AddDependentCompilationInfo(
+          map, DependentCode::kPrototypeCheckGroup, top_info());
+    }
+  }
   return New<HLoadNamedField>(
-      checked_object, checked_object, access, info->field_maps(), top_info());
+      checked_object, checked_object, access, maps, info->field_type());
 }
 
 
@@ -5488,6 +5504,7 @@ bool HOptimizedGraphBuilder::PropertyAccessInfo::IsCompatible(
     }
   }
   info->GeneralizeRepresentation(r);
+  info->field_type_ = info->field_type_.Combine(field_type_);
   return true;
 }
 
@@ -5539,8 +5556,9 @@ bool HOptimizedGraphBuilder::PropertyAccessInfo::LoadResult(Handle<Map> map) {
 
 void HOptimizedGraphBuilder::PropertyAccessInfo::LoadFieldMaps(
     Handle<Map> map) {
-  // Clear any previously collected field maps.
+  // Clear any previously collected field maps/type.
   field_maps_.Clear();
+  field_type_ = HType::Tagged();
 
   // Figure out the field type from the accessor map.
   Handle<HeapType> field_type(lookup_.GetFieldTypeFromMap(*map), isolate());
@@ -5563,6 +5581,22 @@ void HOptimizedGraphBuilder::PropertyAccessInfo::LoadFieldMaps(
   field_maps_.Sort();
   ASSERT_EQ(num_field_maps, field_maps_.length());
 
+  // Determine field HType from field HeapType.
+  if (field_type->Is(HeapType::Number())) {
+    field_type_ = HType::HeapNumber();
+  } else if (field_type->Is(HeapType::String())) {
+    field_type_ = HType::String();
+  } else if (field_type->Is(HeapType::Boolean())) {
+    field_type_ = HType::Boolean();
+  } else if (field_type->Is(HeapType::Array())) {
+    field_type_ = HType::JSArray();
+  } else if (field_type->Is(HeapType::Object())) {
+    field_type_ = HType::JSObject();
+  } else if (field_type->Is(HeapType::Null()) ||
+             field_type->Is(HeapType::Undefined())) {
+    field_type_ = HType::NonPrimitive();
+  }
+
   // Add dependency on the map that introduced the field.
   Map::AddDependentCompilationInfo(
       handle(lookup_.GetFieldOwnerFromMap(*map), isolate()),

diff --git a/src/hydrogen.h b/src/hydrogen.h
index 9dc5741..d20a817 100644 (file)
--- a/src/hydrogen.h
+++ b/src/hydrogen.h
@@ -2326,6 +2326,7 @@ class HOptimizedGraphBuilder : public HGraphBuilder, public AstVisitor {
           access_type_(access_type),
           type_(type),
           name_(name),
+          field_type_(HType::Tagged()),
           access_(HObjectAccess::ForMap()) { }
 
     // Checkes whether this PropertyAccessInfo can be handled as a monomorphic
@@ -2392,6 +2393,7 @@ class HOptimizedGraphBuilder : public HGraphBuilder, public AstVisitor {
     Handle<Object> constant() { return constant_; }
     Handle<Map> transition() { return handle(lookup_.GetTransitionTarget()); }
     SmallMapList* field_maps() { return &field_maps_; }
+    HType field_type() const { return field_type_; }
     HObjectAccess access() { return access_; }
 
    private:
@@ -2422,6 +2424,7 @@ class HOptimizedGraphBuilder : public HGraphBuilder, public AstVisitor {
     Handle<JSObject> api_holder_;
     Handle<Object> constant_;
     SmallMapList field_maps_;
+    HType field_type_;
     HObjectAccess access_;
   };