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

[compiler] Consider CheckMaps with migration as side-effecting

Browse Source 
CheckMaps with migration can (and is expected to) mutate fields and maps
on migration, which means it cannot be considered to be
non-side-effecting in terms of writes.

This allows us to revert crrev.com/c/3998653, as we should now correctly
insert a dynamic map re-check after a potential map migration.

Bug: chromium:1380063
Change-Id: I28f78f0579529279b4c3810fabbd2edb653a6f1b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4203379
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85549}
This commit is contained in:
Leszek Swirski 2023-01-30 15:30:47 +01:00 committed by V8 LUCI CQ
parent e6a2efbc1c
commit cfd4728fb2
2 changed files with 30 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
src/compiler/effect-control-linearizer.cc
View File

@ -5937,8 +5937,6 @@ Node* EffectControlLinearizer::LowerLoadFieldByIndex(Node* node) {
auto if_double = __ MakeDeferredLabel(); auto if_double = __ MakeDeferredLabel();
auto done = __ MakeLabel(MachineRepresentation::kTagged); auto done = __ MakeLabel(MachineRepresentation::kTagged);
auto loaded_field = __ MakeLabel(MachineRepresentation::kTagged);
auto done_double = __ MakeLabel(MachineRepresentation::kFloat64);
// Check if field is a mutable double field. // Check if field is a mutable double field.
__ GotoIfNot(__ IntPtrEqual(__ WordAnd(index, one), zero), &if_double); __ GotoIfNot(__ IntPtrEqual(__ WordAnd(index, one), zero), &if_double);
@ -5955,8 +5953,8 @@ Node* EffectControlLinearizer::LowerLoadFieldByIndex(Node* node) {
Node* offset = Node* offset =
__ IntAdd(__ WordShl(index, __ IntPtrConstant(kTaggedSizeLog2 - 1)), __ IntAdd(__ WordShl(index, __ IntPtrConstant(kTaggedSizeLog2 - 1)),
__ IntPtrConstant(JSObject::kHeaderSize - kHeapObjectTag)); __ IntPtrConstant(JSObject::kHeaderSize - kHeapObjectTag));
Node* field = __ Load(MachineType::AnyTagged(), object, offset); Node* result = __ Load(MachineType::AnyTagged(), object, offset);
__ Goto(&loaded_field, field); __ Goto(&done, result);
} }
// The field is located in the properties backing store of {object}. // The field is located in the properties backing store of {object}.
@ -5970,8 +5968,8 @@ Node* EffectControlLinearizer::LowerLoadFieldByIndex(Node* node) {
__ IntPtrConstant(kTaggedSizeLog2 - 1)), __ IntPtrConstant(kTaggedSizeLog2 - 1)),
__ IntPtrConstant((FixedArray::kHeaderSize - kTaggedSize) - __ IntPtrConstant((FixedArray::kHeaderSize - kTaggedSize) -
kHeapObjectTag)); kHeapObjectTag));
Node* field = __ Load(MachineType::AnyTagged(), properties, offset); Node* result = __ Load(MachineType::AnyTagged(), properties, offset);
__ Goto(&loaded_field, field); __ Goto(&done, result);
} }
} }
@ -5979,6 +5977,9 @@ Node* EffectControlLinearizer::LowerLoadFieldByIndex(Node* node) {
// architectures, or a mutable HeapNumber. // architectures, or a mutable HeapNumber.
__ Bind(&if_double); __ Bind(&if_double);
{ {
auto loaded_field = __ MakeLabel(MachineRepresentation::kTagged);
auto done_double = __ MakeLabel(MachineRepresentation::kFloat64);
index = __ WordSar(index, one); index = __ WordSar(index, one);
// Check if field is in-object or out-of-object. // Check if field is in-object or out-of-object.
@ -6006,27 +6007,27 @@ Node* EffectControlLinearizer::LowerLoadFieldByIndex(Node* node) {
Node* field = __ Load(MachineType::AnyTagged(), properties, offset); Node* field = __ Load(MachineType::AnyTagged(), properties, offset);
__ Goto(&loaded_field, field); __ Goto(&loaded_field, field);
} }
}
__ Bind(&loaded_field); __ Bind(&loaded_field);
{ {
Node* field = loaded_field.PhiAt(0); Node* field = loaded_field.PhiAt(0);
// We may have transitioned in-place away from double, so check that // We may have transitioned in-place away from double, so check that
// this is a HeapNumber -- otherwise the load is fine and we don't need // this is a HeapNumber -- otherwise the load is fine and we don't need
// to copy anything anyway. // to copy anything anyway.
__ GotoIf(ObjectIsSmi(field), &done, field); __ GotoIf(ObjectIsSmi(field), &done, field);
Node* field_map = __ LoadField(AccessBuilder::ForMap(), field); Node* field_map = __ LoadField(AccessBuilder::ForMap(), field);
__ GotoIfNot(__ TaggedEqual(field_map, __ HeapNumberMapConstant()), &done, __ GotoIfNot(__ TaggedEqual(field_map, __ HeapNumberMapConstant()), &done,
field); field);
Node* value = __ LoadField(AccessBuilder::ForHeapNumberValue(), field); Node* value = __ LoadField(AccessBuilder::ForHeapNumberValue(), field);
__ Goto(&done_double, value); __ Goto(&done_double, value);
} }
__ Bind(&done_double); __ Bind(&done_double);
{ {
Node* result = AllocateHeapNumberWithValue(done_double.PhiAt(0)); Node* result = AllocateHeapNumberWithValue(done_double.PhiAt(0));
__ Goto(&done, result); __ Goto(&done, result);
}
} }
__ Bind(&done); __ Bind(&done);

6
src/compiler/simplified-operator.cc
View File

@ -1715,9 +1715,13 @@ const Operator* SimplifiedOperatorBuilder::CheckMaps(
CheckMapsFlags flags, ZoneHandleSet<Map> maps, CheckMapsFlags flags, ZoneHandleSet<Map> maps,
const FeedbackSource& feedback) { const FeedbackSource& feedback) {
CheckMapsParameters const parameters(flags, maps, feedback); CheckMapsParameters const parameters(flags, maps, feedback);
Operator::Properties operator_props = Operator::kNoThrow;
if (!(flags & CheckMapsFlag::kTryMigrateInstance)) {
operator_props |= Operator::kNoWrite;
}
return zone()->New<Operator1<CheckMapsParameters>>( // -- return zone()->New<Operator1<CheckMapsParameters>>( // --
IrOpcode::kCheckMaps, // opcode IrOpcode::kCheckMaps, // opcode
Operator::kNoThrow | Operator::kNoWrite, // flags operator_props, // flags
"CheckMaps", // name "CheckMaps", // name
1, 1, 1, 0, 1, 0, // counts 1, 1, 1, 0, 1, 0, // counts
parameters); // parameter parameters); // parameter