diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index 8c42260460..e23af688e8 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -464,7 +464,10 @@ Reduction JSCallReducer::ReduceObjectPrototypeHasOwnProperty(Node* node) { // We can constant-fold the {node} to True in this case, and insert // a (potentially redundant) map check to guard the fact that the - // {receiver} map didn't change since the dominating JSForInNext. + // {receiver} map didn't change since the dominating JSForInNext. This + // map check is only necessary when TurboFan cannot prove that there + // is no observable side effect between the {JSForInNext} and the + // {JSCall} to Object.prototype.hasOwnProperty. // // Also note that it's safe to look through the {JSToObject}, since the // Object.prototype.hasOwnProperty does an implicit ToObject anyway, and @@ -478,13 +481,17 @@ Reduction JSCallReducer::ReduceObjectPrototypeHasOwnProperty(Node* node) { object = NodeProperties::GetValueInput(object, 0); } if (object == receiver) { - Node* receiver_map = effect = - graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()), - receiver, effect, control); - Node* check = graph()->NewNode(simplified()->ReferenceEqual(), - receiver_map, cache_type); - effect = - graph()->NewNode(simplified()->CheckIf(), check, effect, control); + // No need to repeat the map check if we can prove that there's no + // observable side effect between {effect} and {name]. + if (!NodeProperties::NoObservableSideEffectBetween(effect, name)) { + Node* receiver_map = effect = + graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()), + receiver, effect, control); + Node* check = graph()->NewNode(simplified()->ReferenceEqual(), + receiver_map, cache_type); + effect = + graph()->NewNode(simplified()->CheckIf(), check, effect, control); + } Node* value = jsgraph()->TrueConstant(); ReplaceWithValue(node, value, effect, control); return Replace(value); @@ -1212,12 +1219,9 @@ Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread( // TODO(turbofan): Further relax this constraint. if (formal_parameter_count != 0) { Node* effect = NodeProperties::GetEffectInput(node); - while (effect != arguments_list) { - if (effect->op()->EffectInputCount() != 1 || - !(effect->op()->properties() & Operator::kNoWrite)) { - return NoChange(); - } - effect = NodeProperties::GetEffectInput(effect); + if (!NodeProperties::NoObservableSideEffectBetween(effect, + arguments_list)) { + return NoChange(); } } } else if (type == CreateArgumentsType::kRestParameter) { diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc index 206637025a..650079df1c 100644 --- a/src/compiler/js-native-context-specialization.cc +++ b/src/compiler/js-native-context-specialization.cc @@ -1412,7 +1412,9 @@ Reduction JSNativeContextSpecialization::ReduceJSLoadProperty(Node* node) { // If the for..in has only seen maps with enum cache consisting of keys // and indices so far, we can turn the {JSLoadProperty} into a map check // on the {receiver} and then just load the field value dynamically via - // the {LoadFieldByIndex} operator. + // the {LoadFieldByIndex} operator. The map check is only necessary when + // TurboFan cannot prove that there is no observable side effect between + // the {JSForInNext} and the {JSLoadProperty} node. // // Also note that it's safe to look through the {JSToObject}, since the // [[Get]] operation does an implicit ToObject anyway, and these operations @@ -1427,8 +1429,48 @@ Reduction JSNativeContextSpecialization::ReduceJSLoadProperty(Node* node) { object = NodeProperties::GetValueInput(object, 0); } if (object == receiver) { - Node* value = effect = - BuildForInNextValue(receiver, enumerator, index, effect, control); + // No need to repeat the map check if we can prove that there's no + // observable side effect between {effect} and {name]. + if (!NodeProperties::NoObservableSideEffectBetween(effect, name)) { + // Check that the {receiver} map is still valid. + Node* receiver_map = effect = + graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()), + receiver, effect, control); + Node* check = graph()->NewNode(simplified()->ReferenceEqual(), + receiver_map, enumerator); + effect = + graph()->NewNode(simplified()->CheckIf(), check, effect, control); + } + + // Load the enum cache indices from the {cache_type}. + Node* descriptor_array = effect = graph()->NewNode( + simplified()->LoadField(AccessBuilder::ForMapDescriptors()), + enumerator, effect, control); + Node* enum_cache = effect = + graph()->NewNode(simplified()->LoadField( + AccessBuilder::ForDescriptorArrayEnumCache()), + descriptor_array, effect, control); + Node* enum_indices = effect = graph()->NewNode( + simplified()->LoadField(AccessBuilder::ForEnumCacheIndices()), + enum_cache, effect, control); + + // Ensure that the {enum_indices} are valid. + Node* check = graph()->NewNode( + simplified()->BooleanNot(), + graph()->NewNode(simplified()->ReferenceEqual(), enum_indices, + jsgraph()->EmptyFixedArrayConstant())); + effect = + graph()->NewNode(simplified()->CheckIf(), check, effect, control); + + // Determine the index from the {enum_indices}. + index = effect = graph()->NewNode( + simplified()->LoadElement( + AccessBuilder::ForFixedArrayElement(PACKED_SMI_ELEMENTS)), + enum_indices, index, effect, control); + + // Load the actual field value. + Node* value = effect = graph()->NewNode( + simplified()->LoadFieldByIndex(), receiver, index, effect, control); ReplaceWithValue(node, value, effect, control); return Replace(value); } @@ -2337,48 +2379,6 @@ Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore( return graph()->NewNode(common()->FinishRegion(), new_properties, effect); } -Node* JSNativeContextSpecialization::BuildForInNextValue(Node* receiver, - Node* enumerator, - Node* index, - Node* effect, - Node* control) { - // Check that the {receiver} map is still valid. - Node* receiver_map = effect = - graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()), - receiver, effect, control); - Node* check = graph()->NewNode(simplified()->ReferenceEqual(), receiver_map, - enumerator); - effect = graph()->NewNode(simplified()->CheckIf(), check, effect, control); - - // Load the enum cache indices from the {cache_type}. - Node* descriptor_array = effect = graph()->NewNode( - simplified()->LoadField(AccessBuilder::ForMapDescriptors()), enumerator, - effect, control); - Node* enum_cache = effect = graph()->NewNode( - simplified()->LoadField(AccessBuilder::ForDescriptorArrayEnumCache()), - descriptor_array, effect, control); - Node* enum_indices = effect = graph()->NewNode( - simplified()->LoadField(AccessBuilder::ForEnumCacheIndices()), enum_cache, - effect, control); - - // Ensure that the {enum_indices} are valid. - check = graph()->NewNode( - simplified()->BooleanNot(), - graph()->NewNode(simplified()->ReferenceEqual(), enum_indices, - jsgraph()->EmptyFixedArrayConstant())); - effect = graph()->NewNode(simplified()->CheckIf(), check, effect, control); - - // Determine the index from the {enum_indices}. - index = effect = graph()->NewNode( - simplified()->LoadElement( - AccessBuilder::ForFixedArrayElement(PACKED_SMI_ELEMENTS)), - enum_indices, index, effect, control); - - // Load the actual field value. - return graph()->NewNode(simplified()->LoadFieldByIndex(), receiver, index, - effect, control); -} - bool JSNativeContextSpecialization::CanTreatHoleAsUndefined( MapHandles const& receiver_maps) { // Check if all {receiver_maps} either have one of the initial Array.prototype diff --git a/src/compiler/js-native-context-specialization.h b/src/compiler/js-native-context-specialization.h index f2b5330671..f7e9439e29 100644 --- a/src/compiler/js-native-context-specialization.h +++ b/src/compiler/js-native-context-specialization.h @@ -162,11 +162,6 @@ class JSNativeContextSpecialization final : public AdvancedReducer { Node* BuildExtendPropertiesBackingStore(Handle map, Node* properties, Node* effect, Node* control); - // Construct appropriate subgraph to read the next value in a fast for-in - // iteration. - Node* BuildForInNextValue(Node* receiver, Node* enumerator, Node* index, - Node* effect, Node* control); - // Checks if we can turn the hole into undefined when loading an element // from an object with one of the {receiver_maps}; sets up appropriate // code dependencies and might use the array protector cell. diff --git a/src/compiler/node-properties.cc b/src/compiler/node-properties.cc index 0eafc4a2a7..e81693791a 100644 --- a/src/compiler/node-properties.cc +++ b/src/compiler/node-properties.cc @@ -456,6 +456,20 @@ NodeProperties::InferReceiverMapsResult NodeProperties::InferReceiverMaps( } } +// static +bool NodeProperties::NoObservableSideEffectBetween(Node* effect, + Node* dominator) { + while (effect != dominator) { + if (effect->op()->EffectInputCount() == 1 && + effect->op()->properties() & Operator::kNoWrite) { + effect = NodeProperties::GetEffectInput(effect); + } else { + return false; + } + } + return true; +} + // static Node* NodeProperties::GetOuterContext(Node* node, size_t* depth) { Node* context = NodeProperties::GetContextInput(node); diff --git a/src/compiler/node-properties.h b/src/compiler/node-properties.h index a01a229c64..6bc1fe7078 100644 --- a/src/compiler/node-properties.h +++ b/src/compiler/node-properties.h @@ -150,6 +150,11 @@ class V8_EXPORT_PRIVATE NodeProperties final { static InferReceiverMapsResult InferReceiverMaps( Node* receiver, Node* effect, ZoneHandleSet* maps_return); + // Walks up the {effect} chain to check that there's no observable side-effect + // between the {effect} and it's {dominator}. Aborts the walk if there's join + // in the effect chain. + static bool NoObservableSideEffectBetween(Node* effect, Node* dominator); + // --------------------------------------------------------------------------- // Context.