[turboprop] For double data fields check loaded value is HeapNumber

With in-place representation changes we can transition from a double
field to a Tagged field without changing the map. So it is not always
safe to assume the loaded value would be a HeapNumber. TurboFan takes a
dependency on the field representation to ensure the code is deoptimized
on any changes. With dynamic map checks, it is not possible to take such
a dependency. Hence check the loaded value is a HeapNumber and
deoptimize otherwise.

Bug: chromium:1112155,v8:10582
Change-Id: I910ac1c0781ac8842fdbf272f9173b55b02923ba
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2354810
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69521}

src/compiler/access-info.cc

@@ -40,6 +40,19 @@ bool CanInlinePropertyAccess(Handle<Map> map) {
          !map->is_access_check_needed();
 }
 
+#ifdef DEBUG
+bool HasFieldRepresentationDependenciesOnMap(
+    ZoneVector<CompilationDependency const*>& dependencies,
+    Handle<Map> const& field_owner_map) {
+  for (auto dep : dependencies) {
+    if (dep->IsFieldRepresentationDependencyOnMap(field_owner_map)) {
+      return true;
+    }
+  }
+  return false;
+}
+#endif
+
 }  // namespace
 
 
@@ -84,6 +97,9 @@ PropertyAccessInfo PropertyAccessInfo::DataField(
     FieldIndex field_index, Representation field_representation,
     Type field_type, Handle<Map> field_owner_map, MaybeHandle<Map> field_map,
     MaybeHandle<JSObject> holder, MaybeHandle<Map> transition_map) {
+  DCHECK_IMPLIES(
+      field_representation.IsDouble(),
+      HasFieldRepresentationDependenciesOnMap(dependencies, field_owner_map));
   return PropertyAccessInfo(kDataField, holder, transition_map, field_index,
                             field_representation, field_type, field_owner_map,
                             field_map, {{receiver_map}, zone},

src/compiler/compilation-dependencies.cc

@@ -179,6 +179,13 @@ class FieldRepresentationDependency final : public CompilationDependency {
                                      DependentCode::kFieldRepresentationGroup);
   }
 
+#ifdef DEBUG
+  bool IsFieldRepresentationDependencyOnMap(
+      Handle<Map> const& receiver_map) const override {
+    return owner_.object().equals(receiver_map);
+  }
+#endif
+
  private:
   MapRef owner_;
   InternalIndex descriptor_;

src/compiler/compilation-dependency.h

@@ -22,6 +22,10 @@ class CompilationDependency : public ZoneObject {
 
 #ifdef DEBUG
   virtual bool IsPretenureModeDependency() const { return false; }
+  virtual bool IsFieldRepresentationDependencyOnMap(
+      Handle<Map> const& receiver_map) const {
+    return false;
+  }
 #endif
 };
 

src/compiler/property-access-builder.cc

@@ -210,6 +210,24 @@ Node* PropertyAccessBuilder::BuildLoadDataField(NameRef const& name,
                                           field_access.const_field_info};
       storage = *effect = graph()->NewNode(
           simplified()->LoadField(storage_access), storage, *effect, *control);
+      if (dependencies() == nullptr) {
+        // We expect the loaded value to be a heap number here. With
+        // in-place field representation changes it is possible this is a
+        // no longer a heap number without map transitions. If we haven't taken
+        // a dependency on field representation, we should verify the loaded
+        // value is a heap number.
+        storage = *effect = graph()->NewNode(simplified()->CheckHeapObject(),
+                                             storage, *effect, *control);
+        Node* map = *effect =
+            graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()),
+                             storage, *effect, *control);
+        Node* is_heap_number =
+            graph()->NewNode(simplified()->ReferenceEqual(), map,
+                             jsgraph()->HeapNumberMapConstant());
+        *effect = graph()->NewNode(
+            simplified()->CheckIf(DeoptimizeReason::kNotAHeapNumber),
+            is_heap_number, *effect, *control);
+      }
       field_access.offset = HeapNumber::kValueOffset;
       field_access.name = MaybeHandle<Name>();
     }

test/mjsunit/regress/regress-1112155.js

+// Copyright 2020 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: --dynamic-map-checks --allow-natives-syntax --opt --no-always-opt
+
+function f(v) {
+  return v.b;
+}
+var v = { a: 10, b: 10.23 };
+%PrepareFunctionForOptimization(f);
+f(v);
+%OptimizeFunctionOnNextCall(f);
+f(v);
+assertOptimized(f);
+v.b = {x: 20};
+assertEquals(f(v).x, 20);
+// Must deoptimize because of field-rep changes for field 'b'
+assertUnoptimized(f);