Skip to content

Commit 94c87fe

Browse files
isheludkoCommit Bot
isheludko
authored and
Commit Bot
committed
[ic] Fix handling of +0/-0 when constant field tracking is enabled
... and ensure that runtime behaviour is in sync with the IC code. Bug: chromium:950747, v8:9113 Change-Id: Ied66c9514cbe3a4d75fc71d4fc3b19ea1538f9b2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1561319 Reviewed-by: Toon Verwaest <[email protected]> Commit-Queue: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{#60768}
1 parent b5eb8da commit 94c87fe

File tree

9 files changed

+327
-92
lines changed

9 files changed

+327
-92
lines changed

‎src/code-stub-assembler.cc‎

Lines changed: 54 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -12507,7 +12507,7 @@ Node* CodeStubAssembler::StrictEqual(Node* lhs, Node* rhs,
1250712507
// This algorithm differs from the Strict Equality Comparison Algorithm in its
1250812508
// treatment of signed zeroes and NaNs.
1250912509
void CodeStubAssembler::BranchIfSameValue(Node* lhs, Node* rhs, Label* if_true,
12510-
Label* if_false) {
12510+
Label* if_false, SameValueMode mode) {
1251112511
VARIABLE(var_lhs_value, MachineRepresentation::kFloat64);
1251212512
VARIABLE(var_rhs_value, MachineRepresentation::kFloat64);
1251312513
Label do_fcmp(this);
@@ -12553,10 +12553,12 @@ void CodeStubAssembler::BranchIfSameValue(Node* lhs, Node* rhs, Label* if_true,
1255312553
if_lhsisbigint(this);
1255412554
Node* const lhs_map = LoadMap(lhs);
1255512555
GotoIf(IsHeapNumberMap(lhs_map), &if_lhsisheapnumber);
12556-
Node* const lhs_instance_type = LoadMapInstanceType(lhs_map);
12557-
GotoIf(IsStringInstanceType(lhs_instance_type), &if_lhsisstring);
12558-
Branch(IsBigIntInstanceType(lhs_instance_type), &if_lhsisbigint,
12559-
if_false);
12556+
if (mode != SameValueMode::kNumbersOnly) {
12557+
Node* const lhs_instance_type = LoadMapInstanceType(lhs_map);
12558+
GotoIf(IsStringInstanceType(lhs_instance_type), &if_lhsisstring);
12559+
GotoIf(IsBigIntInstanceType(lhs_instance_type), &if_lhsisbigint);
12560+
}
12561+
Goto(if_false);
1256012562

1256112563
BIND(&if_lhsisheapnumber);
1256212564
{
@@ -12566,53 +12568,62 @@ void CodeStubAssembler::BranchIfSameValue(Node* lhs, Node* rhs, Label* if_true,
1256612568
Goto(&do_fcmp);
1256712569
}
1256812570

12569-
BIND(&if_lhsisstring);
12570-
{
12571-
// Now we can only yield true if {rhs} is also a String
12572-
// with the same sequence of characters.
12573-
GotoIfNot(IsString(rhs), if_false);
12574-
Node* const result = CallBuiltin(Builtins::kStringEqual,
12575-
NoContextConstant(), lhs, rhs);
12576-
Branch(IsTrue(result), if_true, if_false);
12577-
}
12578-
12579-
BIND(&if_lhsisbigint);
12580-
{
12581-
GotoIfNot(IsBigInt(rhs), if_false);
12582-
Node* const result = CallRuntime(Runtime::kBigIntEqualToBigInt,
12583-
NoContextConstant(), lhs, rhs);
12584-
Branch(IsTrue(result), if_true, if_false);
12571+
if (mode != SameValueMode::kNumbersOnly) {
12572+
BIND(&if_lhsisstring);
12573+
{
12574+
// Now we can only yield true if {rhs} is also a String
12575+
// with the same sequence of characters.
12576+
GotoIfNot(IsString(rhs), if_false);
12577+
Node* const result = CallBuiltin(
12578+
Builtins::kStringEqual, NoContextConstant(), lhs, rhs);
12579+
Branch(IsTrue(result), if_true, if_false);
12580+
}
12581+
12582+
BIND(&if_lhsisbigint);
12583+
{
12584+
GotoIfNot(IsBigInt(rhs), if_false);
12585+
Node* const result =
12586+
CallRuntime(Runtime::kBigIntEqualToBigInt,
12587+
NoContextConstant(), lhs, rhs);
12588+
Branch(IsTrue(result), if_true, if_false);
12589+
}
1258512590
}
1258612591
});
1258712592
}
1258812593

1258912594
BIND(&do_fcmp);
1259012595
{
12591-
Node* const lhs_value = var_lhs_value.value();
12592-
Node* const rhs_value = var_rhs_value.value();
12596+
TNode<Float64T> lhs_value = UncheckedCast<Float64T>(var_lhs_value.value());
12597+
TNode<Float64T> rhs_value = UncheckedCast<Float64T>(var_rhs_value.value());
12598+
BranchIfSameNumberValue(lhs_value, rhs_value, if_true, if_false);
12599+
}
12600+
}
1259312601

12594-
Label if_equal(this), if_notequal(this);
12595-
Branch(Float64Equal(lhs_value, rhs_value), &if_equal, &if_notequal);
12602+
void CodeStubAssembler::BranchIfSameNumberValue(TNode<Float64T> lhs_value,
12603+
TNode<Float64T> rhs_value,
12604+
Label* if_true,
12605+
Label* if_false) {
12606+
Label if_equal(this), if_notequal(this);
12607+
Branch(Float64Equal(lhs_value, rhs_value), &if_equal, &if_notequal);
1259612608

12597-
BIND(&if_equal);
12598-
{
12599-
// We still need to handle the case when {lhs} and {rhs} are -0.0 and
12600-
// 0.0 (or vice versa). Compare the high word to
12601-
// distinguish between the two.
12602-
Node* const lhs_hi_word = Float64ExtractHighWord32(lhs_value);
12603-
Node* const rhs_hi_word = Float64ExtractHighWord32(rhs_value);
12604-
12605-
// If x is +0 and y is -0, return false.
12606-
// If x is -0 and y is +0, return false.
12607-
Branch(Word32Equal(lhs_hi_word, rhs_hi_word), if_true, if_false);
12608-
}
12609+
BIND(&if_equal);
12610+
{
12611+
// We still need to handle the case when {lhs} and {rhs} are -0.0 and
12612+
// 0.0 (or vice versa). Compare the high word to
12613+
// distinguish between the two.
12614+
Node* const lhs_hi_word = Float64ExtractHighWord32(lhs_value);
12615+
Node* const rhs_hi_word = Float64ExtractHighWord32(rhs_value);
1260912616

12610-
BIND(&if_notequal);
12611-
{
12612-
// Return true iff both {rhs} and {lhs} are NaN.
12613-
GotoIf(Float64Equal(lhs_value, lhs_value), if_false);
12614-
Branch(Float64Equal(rhs_value, rhs_value), if_false, if_true);
12615-
}
12617+
// If x is +0 and y is -0, return false.
12618+
// If x is -0 and y is +0, return false.
12619+
Branch(Word32Equal(lhs_hi_word, rhs_hi_word), if_true, if_false);
12620+
}
12621+
12622+
BIND(&if_notequal);
12623+
{
12624+
// Return true iff both {rhs} and {lhs} are NaN.
12625+
GotoIf(Float64Equal(lhs_value, lhs_value), if_false);
12626+
Branch(Float64Equal(rhs_value, rhs_value), if_false, if_true);
1261612627
}
1261712628
}
1261812629

‎src/code-stub-assembler.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3118,7 +3118,14 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
31183118
// ECMA#sec-samevalue
31193119
// Similar to StrictEqual except that NaNs are treated as equal and minus zero
31203120
// differs from positive zero.
3121-
void BranchIfSameValue(Node* lhs, Node* rhs, Label* if_true, Label* if_false);
3121+
enum class SameValueMode { kNumbersOnly, kFull };
3122+
void BranchIfSameValue(Node* lhs, Node* rhs, Label* if_true, Label* if_false,
3123+
SameValueMode mode = SameValueMode::kFull);
3124+
// A part of BranchIfSameValue() that handles two double values.
3125+
// Treats NaN == NaN and +0 != -0.
3126+
void BranchIfSameNumberValue(TNode<Float64T> lhs_value,
3127+
TNode<Float64T> rhs_value, Label* if_true,
3128+
Label* if_false);
31223129

31233130
enum HasPropertyLookupMode { kHasProperty, kForInHasProperty };
31243131

‎src/ic/accessor-assembler.cc‎

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1202,23 +1202,23 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
12021202

12031203
BIND(&inobject);
12041204
{
1205-
Node* field_offset = TimesTaggedSize(field_index);
1205+
TNode<IntPtrT> field_offset = Signed(TimesTaggedSize(field_index));
12061206
Label tagged_rep(this), double_rep(this);
12071207
Branch(
12081208
Word32Equal(representation, Int32Constant(Representation::kDouble)),
12091209
&double_rep, &tagged_rep);
12101210
BIND(&double_rep);
12111211
{
1212-
Node* double_value = ChangeNumberToFloat64(value);
1212+
TNode<Float64T> double_value = ChangeNumberToFloat64(value);
12131213
if (FLAG_unbox_double_fields) {
12141214
if (do_transitioning_store) {
12151215
StoreMap(object, object_map);
12161216
} else if (FLAG_track_constant_fields) {
12171217
Label if_mutable(this);
12181218
GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
1219-
Node* current_value =
1220-
LoadObjectField(object, field_offset, MachineType::Float64());
1221-
Branch(Float64Equal(current_value, double_value), &done, slow);
1219+
TNode<Float64T> current_value =
1220+
LoadObjectField<Float64T>(CAST(object), field_offset);
1221+
BranchIfSameNumberValue(current_value, double_value, &done, slow);
12221222
BIND(&if_mutable);
12231223
}
12241224
StoreObjectFieldNoWriteBarrier(object, field_offset, double_value,
@@ -1234,8 +1234,9 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
12341234
if (FLAG_track_constant_fields) {
12351235
Label if_mutable(this);
12361236
GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
1237-
Node* current_value = LoadHeapNumberValue(mutable_heap_number);
1238-
Branch(Float64Equal(current_value, double_value), &done, slow);
1237+
TNode<Float64T> current_value =
1238+
LoadHeapNumberValue(mutable_heap_number);
1239+
BranchIfSameNumberValue(current_value, double_value, &done, slow);
12391240
BIND(&if_mutable);
12401241
}
12411242
StoreHeapNumberValue(mutable_heap_number, double_value);
@@ -1251,9 +1252,10 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
12511252
} else if (FLAG_track_constant_fields) {
12521253
Label if_mutable(this);
12531254
GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
1254-
Node* current_value =
1255-
LoadObjectField(object, field_offset, MachineType::AnyTagged());
1256-
Branch(WordEqual(current_value, value), &done, slow);
1255+
TNode<Object> current_value =
1256+
LoadObjectField(CAST(object), field_offset);
1257+
BranchIfSameValue(current_value, value, &done, slow,
1258+
SameValueMode::kNumbersOnly);
12571259
BIND(&if_mutable);
12581260
}
12591261
StoreObjectField(object, field_offset, value);
@@ -1303,12 +1305,13 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
13031305
{
13041306
Node* mutable_heap_number =
13051307
LoadPropertyArrayElement(properties, backing_store_index);
1306-
Node* double_value = ChangeNumberToFloat64(value);
1308+
TNode<Float64T> double_value = ChangeNumberToFloat64(value);
13071309
if (FLAG_track_constant_fields) {
13081310
Label if_mutable(this);
13091311
GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
1310-
Node* current_value = LoadHeapNumberValue(mutable_heap_number);
1311-
Branch(Float64Equal(current_value, double_value), &done, slow);
1312+
TNode<Float64T> current_value =
1313+
LoadHeapNumberValue(mutable_heap_number);
1314+
BranchIfSameNumberValue(current_value, double_value, &done, slow);
13121315
BIND(&if_mutable);
13131316
}
13141317
StoreHeapNumberValue(mutable_heap_number, double_value);
@@ -1319,9 +1322,10 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
13191322
if (FLAG_track_constant_fields) {
13201323
Label if_mutable(this);
13211324
GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
1322-
Node* current_value =
1325+
TNode<Object> current_value =
13231326
LoadPropertyArrayElement(properties, backing_store_index);
1324-
Branch(WordEqual(current_value, value), &done, slow);
1327+
BranchIfSameValue(current_value, value, &done, slow,
1328+
SameValueMode::kNumbersOnly);
13251329
BIND(&if_mutable);
13261330
}
13271331
StorePropertyArrayElement(properties, backing_store_index, value);
@@ -1813,7 +1817,7 @@ void AccessorAssembler::StoreNamedField(Node* handler_word, Node* object,
18131817
}
18141818

18151819
Node* index = DecodeWord<StoreHandler::FieldIndexBits>(handler_word);
1816-
Node* offset = IntPtrMul(index, IntPtrConstant(kTaggedSize));
1820+
TNode<IntPtrT> offset = Signed(TimesTaggedSize(index));
18171821
if (representation.IsDouble()) {
18181822
if (!FLAG_unbox_double_fields || !is_inobject) {
18191823
// Load the mutable heap number.
@@ -1831,14 +1835,14 @@ void AccessorAssembler::StoreNamedField(Node* handler_word, Node* object,
18311835
&done);
18321836
{
18331837
if (store_value_as_double) {
1834-
Node* current_value =
1835-
LoadObjectField(property_storage, offset, MachineType::Float64());
1836-
GotoIfNot(Float64Equal(current_value, value), bailout);
1838+
TNode<Float64T> current_value =
1839+
LoadObjectField<Float64T>(CAST(property_storage), offset);
1840+
BranchIfSameNumberValue(current_value, UncheckedCast<Float64T>(value),
1841+
&done, bailout);
18371842
} else {
18381843
Node* current_value = LoadObjectField(property_storage, offset);
1839-
GotoIfNot(WordEqual(current_value, value), bailout);
1844+
Branch(WordEqual(current_value, value), &done, bailout);
18401845
}
1841-
Goto(&done);
18421846
}
18431847
BIND(&done);
18441848
}

‎src/lookup.cc‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -934,10 +934,14 @@ bool LookupIterator::IsConstFieldValueEqualTo(Object value) const {
934934
// Uninitialized double field.
935935
return true;
936936
}
937-
return bit_cast<double>(bits) == value->Number();
937+
return Object::SameNumberValue(bit_cast<double>(bits), value->Number());
938938
} else {
939939
Object current_value = holder->RawFastPropertyAt(field_index);
940-
return current_value->IsUninitialized(isolate()) || current_value == value;
940+
if (current_value->IsUninitialized(isolate()) || current_value == value) {
941+
return true;
942+
}
943+
return current_value->IsNumber() && value->IsNumber() &&
944+
Object::SameNumberValue(current_value->Number(), value->Number());
941945
}
942946
}
943947

‎src/objects-inl.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,16 @@ double Object::Number() const {
383383
: HeapNumber::unchecked_cast(*this)->value();
384384
}
385385

386+
// static
387+
bool Object::SameNumberValue(double value1, double value2) {
388+
// SameNumberValue(NaN, NaN) is true.
389+
if (value1 != value2) {
390+
return std::isnan(value1) && std::isnan(value2);
391+
}
392+
// SameNumberValue(0.0, -0.0) is false.
393+
return (std::signbit(value1) == std::signbit(value2));
394+
}
395+
386396
bool Object::IsNaN() const {
387397
return this->IsHeapNumber() && std::isnan(HeapNumber::cast(*this)->value());
388398
}

0 commit comments

Comments
 (0)