Make LoadElimination aware of const fields

Change-Id: I28f2c87ffae32d16bcfb7cb17ec6e607e7fa2285
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1599172
Commit-Queue: Georg Schmid <gsps@google.com>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61506}

src/compiler/access-info.cc

@@ -326,11 +326,11 @@ bool AccessInfoFactory::ComputeElementAccessInfos(
 
 PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
     Handle<Map> receiver_map, Handle<Map> map, MaybeHandle<JSObject> holder,
-    int number, AccessMode access_mode) const {
-  DCHECK_NE(number, DescriptorArray::kNotFound);
+    int descriptor, AccessMode access_mode) const {
+  DCHECK_NE(descriptor, DescriptorArray::kNotFound);
   Handle<DescriptorArray> descriptors(map->instance_descriptors(), isolate());
-  PropertyDetails const details = descriptors->GetDetails(number);
-  int index = descriptors->GetFieldIndex(number);
+  PropertyDetails const details = descriptors->GetDetails(descriptor);
+  int index = descriptors->GetFieldIndex(descriptor);
   Representation details_representation = details.representation();
   if (details_representation.IsNone()) {
     // The ICs collect feedback in PREMONOMORPHIC state already,
@@ -347,6 +347,7 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
       MachineType::RepCompressedTagged();
   MaybeHandle<Map> field_map;
   MapRef map_ref(broker(), map);
+  map_ref.SerializeOwnDescriptors();  // TODO(neis): Remove later.
   ZoneVector<CompilationDependencies::Dependency const*>
       unrecorded_dependencies(zone());
   if (details_representation.IsSmi()) {
@@ -355,7 +356,7 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
     map_ref.SerializeOwnDescriptors();  // TODO(neis): Remove later.
     unrecorded_dependencies.push_back(
         dependencies()->FieldRepresentationDependencyOffTheRecord(map_ref,
-                                                                  number));
+                                                                  descriptor));
   } else if (details_representation.IsDouble()) {
     field_type = type_cache_->kFloat64;
     field_representation = MachineRepresentation::kFloat64;
@@ -363,8 +364,8 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
     // Extract the field type from the property details (make sure its
     // representation is TaggedPointer to reflect the heap object case).
     field_representation = MachineType::RepCompressedTaggedPointer();
-    Handle<FieldType> descriptors_field_type(descriptors->GetFieldType(number),
-                                             isolate());
+    Handle<FieldType> descriptors_field_type(
+        descriptors->GetFieldType(descriptor), isolate());
     if (descriptors_field_type->IsNone()) {
       // Store is not safe if the field type was cleared.
       if (access_mode == AccessMode::kStore) {
@@ -377,17 +378,20 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
     map_ref.SerializeOwnDescriptors();  // TODO(neis): Remove later.
     unrecorded_dependencies.push_back(
         dependencies()->FieldRepresentationDependencyOffTheRecord(map_ref,
-                                                                  number));
+                                                                  descriptor));
     if (descriptors_field_type->IsClass()) {
       unrecorded_dependencies.push_back(
-          dependencies()->FieldTypeDependencyOffTheRecord(map_ref, number));
+          dependencies()->FieldTypeDependencyOffTheRecord(map_ref, descriptor));
       // Remember the field map, and try to infer a useful type.
       Handle<Map> map(descriptors_field_type->AsClass(), isolate());
       field_type = Type::For(MapRef(broker(), map));
       field_map = MaybeHandle<Map>(map);
     }
   }
-  switch (details.constness()) {
+  map_ref.SerializeOwnDescriptors();  // TODO(neis): Remove later.
+  PropertyConstness constness =
+      dependencies()->DependOnFieldConstness(map_ref, descriptor);
+  switch (constness) {
     case PropertyConstness::kMutable:
       return PropertyAccessInfo::DataField(
           zone(), receiver_map, std::move(unrecorded_dependencies), field_index,
@@ -402,10 +406,11 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
 
 PropertyAccessInfo AccessInfoFactory::ComputeAccessorDescriptorAccessInfo(
     Handle<Map> receiver_map, Handle<Name> name, Handle<Map> map,
-    MaybeHandle<JSObject> holder, int number, AccessMode access_mode) const {
-  DCHECK_NE(number, DescriptorArray::kNotFound);
+    MaybeHandle<JSObject> holder, int descriptor,
+    AccessMode access_mode) const {
+  DCHECK_NE(descriptor, DescriptorArray::kNotFound);
   Handle<DescriptorArray> descriptors(map->instance_descriptors(), isolate());
-  SLOW_DCHECK(number == descriptors->Search(*name, *map));
+  SLOW_DCHECK(descriptor == descriptors->Search(*name, *map));
   if (map->instance_type() == JS_MODULE_NAMESPACE_TYPE) {
     DCHECK(map->is_prototype_map());
     Handle<PrototypeInfo> proto_info(PrototypeInfo::cast(map->prototype_info()),
@@ -427,7 +432,7 @@ PropertyAccessInfo AccessInfoFactory::ComputeAccessorDescriptorAccessInfo(
     return PropertyAccessInfo::AccessorConstant(zone(), receiver_map,
                                                 Handle<Object>(), holder);
   }
-  Handle<Object> accessors(descriptors->GetStrongValue(number), isolate());
+  Handle<Object> accessors(descriptors->GetStrongValue(descriptor), isolate());
   if (!accessors->IsAccessorPair()) {
     return PropertyAccessInfo::Invalid(zone());
   }

src/compiler/access-info.h

@@ -204,11 +204,12 @@ class AccessInfoFactory final {
   PropertyAccessInfo ComputeDataFieldAccessInfo(Handle<Map> receiver_map,
                                                 Handle<Map> map,
                                                 MaybeHandle<JSObject> holder,
-                                                int number,
+                                                int descriptor,
                                                 AccessMode access_mode) const;
   PropertyAccessInfo ComputeAccessorDescriptorAccessInfo(
       Handle<Map> receiver_map, Handle<Name> name, Handle<Map> map,
-      MaybeHandle<JSObject> holder, int number, AccessMode access_mode) const;
+      MaybeHandle<JSObject> holder, int descriptor,
+      AccessMode access_mode) const;
 
   void MergePropertyAccessInfos(ZoneVector<PropertyAccessInfo> infos,
                                 AccessMode access_mode,

src/compiler/js-native-context-specialization.cc

@@ -459,20 +459,6 @@ Reduction JSNativeContextSpecialization::ReduceJSInstanceOf(Node* node) {
       return NoChange();
     }
 
-    // Install dependency on constness. Unfortunately, access_info does not
-    // track descriptor index, so we have to search for it.
-    MapRef holder_map(broker(), handle(holder->map(), isolate()));
-    Handle<DescriptorArray> descriptors(
-        holder_map.object()->instance_descriptors(), isolate());
-    int descriptor_index = descriptors->Search(
-        *(factory()->has_instance_symbol()), *(holder_map.object()));
-    CHECK_NE(descriptor_index, DescriptorArray::kNotFound);
-    holder_map.SerializeOwnDescriptors();
-    if (dependencies()->DependOnFieldConstness(holder_map, descriptor_index) !=
-        PropertyConstness::kConst) {
-      return NoChange();
-    }
-
     if (found_on_proto) {
       dependencies()->DependOnStablePrototypeChains(
           access_info.receiver_maps(), kStartAtPrototype,

src/compiler/load-elimination.h

@@ -182,11 +182,7 @@ class V8_EXPORT_PRIVATE LoadElimination final
 
   class AbstractState final : public ZoneObject {
    public:
-    AbstractState() {
-      for (size_t i = 0; i < arraysize(fields_); ++i) {
-        fields_[i] = nullptr;
-      }
-    }
+    AbstractState() {}
 
     bool Equals(AbstractState const* that) const;
     void Merge(AbstractState const* that, Zone* zone);
@@ -199,7 +195,9 @@ class V8_EXPORT_PRIVATE LoadElimination final
     bool LookupMaps(Node* object, ZoneHandleSet<Map>* object_maps) const;
 
     AbstractState const* AddField(Node* object, size_t index, Node* value,
-                                  MaybeHandle<Name> name, Zone* zone) const;
+                                  MaybeHandle<Name> name,
+                                  PropertyConstness constness,
+                                  Zone* zone) const;
     AbstractState const* KillField(const AliasStateInfo& alias_info,
                                    size_t index, MaybeHandle<Name> name,
                                    Zone* zone) const;
@@ -207,7 +205,9 @@ class V8_EXPORT_PRIVATE LoadElimination final
                                    MaybeHandle<Name> name, Zone* zone) const;
     AbstractState const* KillFields(Node* object, MaybeHandle<Name> name,
                                     Zone* zone) const;
-    Node* LookupField(Node* object, size_t index) const;
+    AbstractState const* KillAll(Zone* zone) const;
+    Node* LookupField(Node* object, size_t index,
+                      PropertyConstness constness) const;
 
     AbstractState const* AddElement(Node* object, Node* index, Node* value,
                                     MachineRepresentation representation,
@@ -219,9 +219,21 @@ class V8_EXPORT_PRIVATE LoadElimination final
 
     void Print() const;
 
+    static AbstractState const* empty_state() { return &empty_state_; }
+
    private:
+    static AbstractState const empty_state_;
+
+    using AbstractFields = std::array<AbstractField const*, kMaxTrackedFields>;
+
+    bool FieldsEquals(AbstractFields const& this_fields,
+                      AbstractFields const& that_fields) const;
+    void FieldsMerge(AbstractFields& this_fields,
+                     AbstractFields const& that_fields, Zone* zone);
+
     AbstractElements const* elements_ = nullptr;
-    AbstractField const* fields_[kMaxTrackedFields];
+    AbstractFields fields_{};
+    AbstractFields const_fields_{};
     AbstractMaps const* maps_ = nullptr;
   };
 
@@ -266,15 +278,17 @@ class V8_EXPORT_PRIVATE LoadElimination final
   static int FieldIndexOf(int offset);
   static int FieldIndexOf(FieldAccess const& access);
 
+  static AbstractState const* empty_state() {
+    return AbstractState::empty_state();
+  }
+
   CommonOperatorBuilder* common() const;
-  AbstractState const* empty_state() const { return &empty_state_; }
   Isolate* isolate() const;
   Factory* factory() const;
   Graph* graph() const;
   JSGraph* jsgraph() const { return jsgraph_; }
   Zone* zone() const { return node_states_.zone(); }
 
-  AbstractState const empty_state_;
   AbstractStateForEffectNodes node_states_;
   JSGraph* const jsgraph_;
 

src/compiler/property-access-builder.cc

@@ -228,17 +228,7 @@ Node* PropertyAccessBuilder::TryBuildLoadConstantDataField(
     if (it.IsReadOnly() && !it.IsConfigurable()) {
       return jsgraph()->Constant(JSReceiver::GetDataProperty(&it));
     } else if (access_info.IsDataConstant()) {
-      // It's necessary to add dependency on the map that introduced
-      // the field.
-      DCHECK(access_info.IsDataConstant());
       DCHECK(!it.is_dictionary_holder());
-      MapRef map(broker(),
-                 handle(it.GetHolder<HeapObject>()->map(), isolate()));
-      map.SerializeOwnDescriptors();  // TODO(neis): Remove later.
-      if (dependencies()->DependOnFieldConstness(
-              map, it.GetFieldDescriptorIndex()) != PropertyConstness::kConst) {
-        return nullptr;
-      }
       return jsgraph()->Constant(JSReceiver::GetDataProperty(&it));
     }
   }
@@ -264,6 +254,9 @@ Node* PropertyAccessBuilder::BuildLoadDataField(
         simplified()->LoadField(AccessBuilder::ForJSObjectPropertiesOrHash()),
         storage, *effect, *control);
   }
+  PropertyConstness constness = access_info.IsDataConstant()
+                                    ? PropertyConstness::kConst
+                                    : PropertyConstness::kMutable;
   FieldAccess field_access = {
       kTaggedBase,
       field_index.offset(),
@@ -272,15 +265,21 @@ Node* PropertyAccessBuilder::BuildLoadDataField(
       field_type,
       MachineType::TypeForRepresentation(field_representation),
       kFullWriteBarrier,
-      LoadSensitivity::kCritical};
+      LoadSensitivity::kCritical,
+      constness};
   if (field_representation == MachineRepresentation::kFloat64) {
     if (!field_index.is_inobject() || field_index.is_hidden_field() ||
         !FLAG_unbox_double_fields) {
       FieldAccess const storage_access = {
-          kTaggedBase,           field_index.offset(),
-          name.object(),         MaybeHandle<Map>(),
-          Type::OtherInternal(), MachineType::TypeCompressedTaggedPointer(),
-          kPointerWriteBarrier,  LoadSensitivity::kCritical};
+          kTaggedBase,
+          field_index.offset(),
+          name.object(),
+          MaybeHandle<Map>(),
+          Type::OtherInternal(),
+          MachineType::TypeCompressedTaggedPointer(),
+          kPointerWriteBarrier,
+          LoadSensitivity::kCritical,
+          constness};
       storage = *effect = graph()->NewNode(
           simplified()->LoadField(storage_access), storage, *effect, *control);
       field_access.offset = HeapNumber::kValueOffset;

src/compiler/simplified-operator.h

@@ -56,6 +56,7 @@ struct FieldAccess {
   MachineType machine_type;       // machine type of the field.
   WriteBarrierKind write_barrier_kind;  // write barrier hint.
   LoadSensitivity load_sensitivity;     // load safety for poisoning.
+  PropertyConstness constness;  // whether the field is assigned only once
 
   FieldAccess()
       : base_is_tagged(kTaggedBase),
@@ -63,12 +64,14 @@ struct FieldAccess {
         type(Type::None()),
         machine_type(MachineType::None()),
         write_barrier_kind(kFullWriteBarrier),
-        load_sensitivity(LoadSensitivity::kUnsafe) {}
+        load_sensitivity(LoadSensitivity::kUnsafe),
+        constness(PropertyConstness::kMutable) {}
 
   FieldAccess(BaseTaggedness base_is_tagged, int offset, MaybeHandle<Name> name,
               MaybeHandle<Map> map, Type type, MachineType machine_type,
               WriteBarrierKind write_barrier_kind,
-              LoadSensitivity load_sensitivity = LoadSensitivity::kUnsafe)
+              LoadSensitivity load_sensitivity = LoadSensitivity::kUnsafe,
+              PropertyConstness constness = PropertyConstness::kMutable)
       : base_is_tagged(base_is_tagged),
         offset(offset),
         name(name),
@@ -76,7 +79,8 @@ struct FieldAccess {
         type(type),
         machine_type(machine_type),
         write_barrier_kind(write_barrier_kind),
-        load_sensitivity(load_sensitivity) {}
+        load_sensitivity(load_sensitivity),
+        constness(constness) {}
 
   int tag() const { return base_is_tagged == kTaggedBase ? kHeapObjectTag : 0; }
 };

test/mjsunit/compiler/load-elimination-const-field.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+// Check that load elimination on const-marked fields works
+(function() {
+    function maybe_sideeffect(b) { return 42; }
+
+    function f(k) {
+        let b = { value: k };
+        maybe_sideeffect(b);
+        let v1 = b.value;
+        maybe_sideeffect(b);
+        let v2 = b.value;
+        %TurbofanStaticAssert(v1 == v2);
+        // TODO(gsps): Improve analysis to also propagate stored value
+        //   Eventually, this should also work:
+        // %TurbofanStaticAssert(v2 == k);
+    }
+
+    %NeverOptimizeFunction(maybe_sideeffect);
+    f(1);
+    f(2);
+    %OptimizeFunctionOnNextCall(f);
+    f(3);
+})();