AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

Revert "Reland "[ic] In-place Double -> Tagged transitions""

Browse Source 
This reverts commit 981aafaf97.

Reason for revert: Still crashing on Canary.

Original change's description:
> Reland "[ic] In-place Double -> Tagged transitions"
>
> This is a reland of 0736599a69.
> This is a reland of 7e1fbe8f34.
>
> 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 <tebbi@chromium.org>
> > Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> > Commit-Queue: Leszek Swirski <leszeks@chromium.org>
> > 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 <leszeks@chromium.org>
> Commit-Queue: Leszek Swirski <leszeks@chromium.org>
> 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 <leszeks@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63550}
This commit is contained in:
Leszek Swirski 2019-09-04 10:58:07 +00:00 committed by Commit Bot
parent e6cc1fc025
commit b293533ed8
8 changed files with 15 additions and 201 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/codegen/code-stub-assembler.h
View File

@ -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 <typename BitField>
TNode<BoolT> IsEqualInWord32(TNode<Word32T> word32,
typename BitField::FieldType value) {
TNode<Word32T> 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 <typename T>
TNode<BoolT> IsSetWord(SloppyTNode<WordT> word) {

11
src/compiler/access-info.cc
View File

@ -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).

39
src/ic/accessor-assembler.cc
View File

@ -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<IntPtrT> index =
@ -262,13 +261,8 @@ void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,
var_double_value->Bind(
LoadObjectField(holder, offset, MachineType::Float64()));
} else {
TNode<Object> 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<HeapNumber> 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<IntPtrT> descriptor_entry =
Signed(DecodeWord<StoreHandler::DescriptorBits>(handler_word));
TNode<DescriptorArray> descriptors = LoadMapDescriptors(LoadMap(holder));
TNode<Uint32T> details =
LoadDetailsByDescriptorEntry(descriptors, descriptor_entry);
GotoIfNot(IsEqualInWord32<PropertyDetails::RepresentationField>(
details, Representation::kDouble),
bailout);
} else if (representation.IsHeapObject()) {
GotoIf(TaggedIsSmi(value), bailout);

2
src/ic/accessor-assembler.h
View File

@ -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<Context> expected_native_context,
TNode<Context> context, Node* receiver,

3
src/objects/map.cc
View File

@ -791,8 +791,7 @@ void Map::GeneralizeField(Isolate* isolate, Handle<Map> 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<Object>(), new_field_type,
MaybeHandle<Object>());
}

4
src/objects/property-details.h
View File

@ -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 {

20
test/cctest/test-field-type-tracking.cc
View File

@ -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);
}
////////////////////////////////////////////////////////////////////////////////

127
test/mjsunit/regress/regress-997485.js
View File

@ -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();
})();