From b293533ed8bf2e54f396164e0cc0ffa7a8bb86e5 Mon Sep 17 00:00:00 2001 From: Leszek Swirski Date: Wed, 4 Sep 2019 10:58:07 +0000 Subject: [PATCH] Revert "Reland "[ic] In-place Double -> Tagged transitions"" This reverts commit 981aafaf977d4671763c341102a4ee5ef172f7f6. Reason for revert: Still crashing on Canary. Original change's description: > Reland "[ic] In-place Double -> Tagged transitions" > > This is a reland of 0736599a69d4d7c145bff2d99fed8a7ea599f687. > This is a reland of 7e1fbe8f341161ca57f95f01b263643d139eb14a. > > Original change description: > > [ic] In-place Double -> Tagged transitions > > > > With no more MutableHeapNumber, we can make Double -> Tagged transitions > > in-place, at the cost of an extra map check when accessing double fields > > to make sure they are still doubles. > > > > Bug: v8:9606 > > Change-Id: I74ff39ed6fba62ee223cd37dfe761f7d73020e1c > > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1743973 > > Reviewed-by: Tobias Tebbi > > Reviewed-by: Toon Verwaest > > Commit-Queue: Leszek Swirski > > Cr-Commit-Position: refs/heads/master@{#63374} > > TBR=verwaest@chromium.org, tebbi@chromium.org > > Bug: v8:9606 > Change-Id: I2d1b7416064d743582f4983fb868316b7e8a4cf2 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1777661 > Reviewed-by: Leszek Swirski > Commit-Queue: Leszek Swirski > Cr-Commit-Position: refs/heads/master@{#63499} TBR=leszeks@chromium.org, verwaest@chromium.org, tebbi@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: v8:9606 Bug: chromium:997989 Change-Id: Ic95166e67df68e84a524dffd8155121c3ff6aa13 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1784283 Commit-Queue: Leszek Swirski Reviewed-by: Leszek Swirski Cr-Commit-Position: refs/heads/master@{#63550} --- src/codegen/code-stub-assembler.h | 10 -- src/compiler/access-info.cc | 11 -- src/ic/accessor-assembler.cc | 39 +------- src/ic/accessor-assembler.h | 2 +- src/objects/map.cc | 3 +- src/objects/property-details.h | 4 +- test/cctest/test-field-type-tracking.cc | 20 ++-- test/mjsunit/regress/regress-997485.js | 127 ------------------------ 8 files changed, 15 insertions(+), 201 deletions(-) delete mode 100644 test/mjsunit/regress/regress-997485.js diff --git a/src/codegen/code-stub-assembler.h b/src/codegen/code-stub-assembler.h index e49e411da3..9884d04e66 100644 --- a/src/codegen/code-stub-assembler.h +++ b/src/codegen/code-stub-assembler.h @@ -2715,16 +2715,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler return Word32Equal(Word32And(word32, const_mask), const_mask); } - // Returns true if the bit field |BitField| in |word32| is equal to a given. - // constant |value|. Avoids a shift compared to using DecodeWord32. - template - TNode IsEqualInWord32(TNode word32, - typename BitField::FieldType value) { - TNode masked_word32 = - Word32And(word32, Int32Constant(BitField::kMask)); - return Word32Equal(masked_word32, Int32Constant(BitField::encode(value))); - } - // Returns true if any of the |T|'s bits in given |word| are set. template TNode IsSetWord(SloppyTNode word) { diff --git a/src/compiler/access-info.cc b/src/compiler/access-info.cc index 0744138ae8..269ef90375 100644 --- a/src/compiler/access-info.cc +++ b/src/compiler/access-info.cc @@ -351,11 +351,6 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo( descriptor)); } else if (details_representation.IsDouble()) { field_type = type_cache_->kFloat64; - if (!FLAG_unbox_double_fields) { - unrecorded_dependencies.push_back( - dependencies()->FieldRepresentationDependencyOffTheRecord( - map_ref, descriptor)); - } } else if (details_representation.IsHeapObject()) { // Extract the field type from the property details (make sure its // representation is TaggedPointer to reflect the heap object case). @@ -794,12 +789,6 @@ PropertyAccessInfo AccessInfoFactory::LookupTransition( transition_map_ref, number)); } else if (details_representation.IsDouble()) { field_type = type_cache_->kFloat64; - if (!FLAG_unbox_double_fields) { - transition_map_ref.SerializeOwnDescriptor(number); - unrecorded_dependencies.push_back( - dependencies()->FieldRepresentationDependencyOffTheRecord( - transition_map_ref, number)); - } } else if (details_representation.IsHeapObject()) { // Extract the field type from the property details (make sure its // representation is TaggedPointer to reflect the heap object case). diff --git a/src/ic/accessor-assembler.cc b/src/ic/accessor-assembler.cc index a80fe20401..f9efcba05f 100644 --- a/src/ic/accessor-assembler.cc +++ b/src/ic/accessor-assembler.cc @@ -16,7 +16,6 @@ #include "src/objects/heap-number.h" #include "src/objects/module.h" #include "src/objects/objects-inl.h" -#include "src/objects/property-details.h" #include "src/objects/smi.h" namespace v8 { @@ -240,7 +239,7 @@ void AccessorAssembler::HandleLoadAccessor( void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word, Variable* var_double_value, - Label* rebox_double, Label* miss, + Label* rebox_double, ExitPoint* exit_point) { Comment("field_load"); TNode index = @@ -262,13 +261,8 @@ void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word, var_double_value->Bind( LoadObjectField(holder, offset, MachineType::Float64())); } else { - TNode heap_number = LoadObjectField(holder, offset); - // This is not an "old" Smi value from before a Smi->Double transition. - // Rather, it's possible that since the last update of this IC, the Double - // field transitioned to a Tagged field, and was then assigned a Smi. - GotoIf(TaggedIsSmi(heap_number), miss); - GotoIfNot(IsHeapNumber(CAST(heap_number)), miss); - var_double_value->Bind(LoadHeapNumberValue(CAST(heap_number))); + TNode heap_number = CAST(LoadObjectField(holder, offset)); + var_double_value->Bind(LoadHeapNumberValue(heap_number)); } Goto(rebox_double); } @@ -282,13 +276,6 @@ void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word, exit_point->Return(value); BIND(&is_double); - if (!FLAG_unbox_double_fields) { - // This is not an "old" Smi value from before a Smi->Double transition. - // Rather, it's possible that since the last update of this IC, the Double - // field transitioned to a Tagged field, and was then assigned a Smi. - GotoIf(TaggedIsSmi(value), miss); - GotoIfNot(IsHeapNumber(CAST(value)), miss); - } var_double_value->Bind(LoadHeapNumberValue(CAST(value))); Goto(rebox_double); } @@ -478,7 +465,7 @@ void AccessorAssembler::HandleLoadICSmiHandlerLoadNamedCase( &module_export, &interceptor); BIND(&field); - HandleLoadField(holder, handler_word, var_double_value, rebox_double, miss, + HandleLoadField(holder, handler_word, var_double_value, rebox_double, exit_point); BIND(&nonexistent); @@ -1696,24 +1683,6 @@ Node* AccessorAssembler::PrepareValueForStore(Node* handler_word, Node* holder, if (representation.IsDouble()) { value = TryTaggedToFloat64(value, bailout); - // We have to check that the representation is still Double. Checking the - // value is nor enough, as we could have transitioned to Tagged but still - // be holding a HeapNumber, which would no longer be allowed to be mutable. - - // TODO(leszeks): We could skip the representation check in favor of a - // constant value check in StoreNamedField here, but then StoreNamedField - // would need an IsHeapNumber check in case both the representation changed - // and the value is no longer a HeapNumber. - TNode descriptor_entry = - Signed(DecodeWord(handler_word)); - TNode descriptors = LoadMapDescriptors(LoadMap(holder)); - TNode details = - LoadDetailsByDescriptorEntry(descriptors, descriptor_entry); - - GotoIfNot(IsEqualInWord32( - details, Representation::kDouble), - bailout); - } else if (representation.IsHeapObject()) { GotoIf(TaggedIsSmi(value), bailout); diff --git a/src/ic/accessor-assembler.h b/src/ic/accessor-assembler.h index f83d8ed5c3..0de2292fd6 100644 --- a/src/ic/accessor-assembler.h +++ b/src/ic/accessor-assembler.h @@ -275,7 +275,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler { void HandleLoadField(Node* holder, Node* handler_word, Variable* var_double_value, Label* rebox_double, - Label* miss, ExitPoint* exit_point); + ExitPoint* exit_point); void EmitAccessCheck(TNode expected_native_context, TNode context, Node* receiver, diff --git a/src/objects/map.cc b/src/objects/map.cc index b0713d2c2e..a672d6580a 100644 --- a/src/objects/map.cc +++ b/src/objects/map.cc @@ -791,8 +791,7 @@ void Map::GeneralizeField(Isolate* isolate, Handle map, int modify_index, map->PrintGeneralization( isolate, stdout, "field type generalization", modify_index, map->NumberOfOwnDescriptors(), map->NumberOfOwnDescriptors(), false, - details.representation(), - descriptors->GetDetails(modify_index).representation(), old_constness, + details.representation(), details.representation(), old_constness, new_constness, old_field_type, MaybeHandle(), new_field_type, MaybeHandle()); } diff --git a/src/objects/property-details.h b/src/objects/property-details.h index a82ddfea23..e350fe2c27 100644 --- a/src/objects/property-details.h +++ b/src/objects/property-details.h @@ -112,9 +112,7 @@ class Representation { // smi and tagged values. Doubles, however, would require a box allocation. if (IsNone()) return !other.IsDouble(); if (!FLAG_modify_field_representation_inplace) return false; - return (IsSmi() || (!FLAG_unbox_double_fields && IsDouble()) || - IsHeapObject()) && - other.IsTagged(); + return (IsSmi() || IsHeapObject()) && other.IsTagged(); } bool is_more_general_than(const Representation& other) const { diff --git a/test/cctest/test-field-type-tracking.cc b/test/cctest/test-field-type-tracking.cc index 8bc81647e4..512bf2a9c6 100644 --- a/test/cctest/test-field-type-tracking.cc +++ b/test/cctest/test-field-type-tracking.cc @@ -641,7 +641,7 @@ void TestGeneralizeField(int detach_property_at_index, int property_index, from.representation, from.type); } else { map = expectations.AddDataField(map, NONE, PropertyConstness::kConst, - Representation::Smi(), any_type); + Representation::Double(), any_type); if (i == detach_property_at_index) { detach_point_map = map; } @@ -654,10 +654,10 @@ void TestGeneralizeField(int detach_property_at_index, int property_index, if (is_detached_map) { detach_point_map = Map::ReconfigureProperty( isolate, detach_point_map, detach_property_at_index, kData, NONE, - Representation::Double(), any_type); + Representation::Tagged(), any_type); expectations.SetDataField(detach_property_at_index, PropertyConstness::kConst, - Representation::Double(), any_type); + Representation::Tagged(), any_type); CHECK(map->is_deprecated()); CHECK(expectations.Check(*detach_point_map, detach_point_map->NumberOfOwnDescriptors())); @@ -814,9 +814,7 @@ TEST(GeneralizeDoubleFieldToTagged) { TestGeneralizeField( {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, - {PropertyConstness::kMutable, Representation::Tagged(), any_type}, - FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace, - !FLAG_unbox_double_fields && FLAG_modify_field_representation_inplace); + {PropertyConstness::kMutable, Representation::Tagged(), any_type}); } TEST(GeneralizeHeapObjectFieldToTagged) { @@ -2299,7 +2297,7 @@ TEST(ElementsKindTransitionFromMapOwningDescriptor) { {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}, - FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace); + true); } } @@ -2367,7 +2365,7 @@ TEST(ElementsKindTransitionFromMapNotOwningDescriptor) { {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}, - FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace); + true); } } @@ -2410,8 +2408,7 @@ TEST(PrototypeTransitionFromMapOwningDescriptor) { TestGeneralizeFieldWithSpecialTransition( config, {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, - {PropertyConstness::kMutable, Representation::Tagged(), any_type}, - FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace); + {PropertyConstness::kMutable, Representation::Tagged(), any_type}, true); } TEST(PrototypeTransitionFromMapNotOwningDescriptor) { @@ -2464,8 +2461,7 @@ TEST(PrototypeTransitionFromMapNotOwningDescriptor) { TestGeneralizeFieldWithSpecialTransition( config, {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, - {PropertyConstness::kMutable, Representation::Tagged(), any_type}, - FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace); + {PropertyConstness::kMutable, Representation::Tagged(), any_type}, true); } //////////////////////////////////////////////////////////////////////////////// diff --git a/test/mjsunit/regress/regress-997485.js b/test/mjsunit/regress/regress-997485.js deleted file mode 100644 index bcc1664222..0000000000 --- a/test/mjsunit/regress/regress-997485.js +++ /dev/null @@ -1,127 +0,0 @@ -// 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 - -(function doubleToTaggedWithTaggedValueStoresCorrectly() { - - function setX_Double(o) { o.x = 4.2; } - - function foo() { - // o.x starts off as Double - const o = { x: 0.1 }; - - // Write to it a few times with setX_Double, to make sure setX_Double has - // Double feedback. - setX_Double(o); - setX_Double(o); - - // Transition o.x to Tagged. - o.x = {}; - - // setX_Double will still have Double feedback, so make sure it works with - // the new Tagged representation o.x. - setX_Double(o); - - assertEquals(o.x, 4.2); - } - - %EnsureFeedbackVectorForFunction(setX_Double); - foo(); - -})(); - -(function doubleToTaggedWithDoubleValueDoesNotMutate() { - - function setX_Double(o) { o.x = 4.2; } - - function foo() { - // o.x starts off as Double - const o = { x: 0.1 }; - - // Write to it a few times with setX_Double, to make sure setX_Double has - // Double feedback. - setX_Double(o); - setX_Double(o); - - // Transition o.x to Tagged. - o.x = {}; - - // Write the HeapNumber val to o.x. - const val = 1.25; - o.x = val; - - // setX_Double will still have Double feedback, which expects to be able to - // mutate o.x's HeapNumber, so make sure it does not mutate val. - setX_Double(o); - - assertEquals(o.x, 4.2); - assertNotEquals(val, 4.2); - } - - %EnsureFeedbackVectorForFunction(setX_Double); - foo(); - -})(); - -(function doubleToTaggedWithTaggedValueStoresSmiCorrectly() { - - function setX_Smi(o) { o.x = 42; } - - function foo() { - // o.x starts off as Double - const o = { x: 0.1 }; - - // Write to it a few times with setX_Smi, to make sure setX_Smi has - // Double feedback. - setX_Smi(o); - setX_Smi(o); - - // Transition o.x to Tagged. - o.x = {}; - - // setX_Smi will still have Double feedback, so make sure it works with - // the new Tagged representation o.x. - setX_Smi(o); - - assertEquals(o.x, 42); - } - - %EnsureFeedbackVectorForFunction(setX_Smi); - foo(); - -})(); - -(function doubleToTaggedWithSmiValueDoesNotMutate() { - - function setX_Smi(o) { o.x = 42; } - - function foo() { - // o.x starts off as Double - const o = { x: 0.1 }; - - // Write to it a few times with setX_Smi, to make sure setX_Smi has - // Double feedback. - setX_Smi(o); - setX_Smi(o); - - // Transition o.x to Tagged. - o.x = {}; - - // Write the HeapNumber val to o.x. - const val = 1.25; - o.x = val; - - // setX_Smi will still have Double feedback, which expects to be able to - // mutate o.x's HeapNumber, so make sure it does not mutate val. - setX_Smi(o); - - assertEquals(o.x, 42); - assertNotEquals(val, 42); - } - - %EnsureFeedbackVectorForFunction(setX_Smi); - foo(); - -})();