From 72be2d2138209eed2959c10cff1f90b9d7b4bc67 Mon Sep 17 00:00:00 2001 From: Tobias Tebbi Date: Mon, 15 Jan 2018 17:32:44 +0100 Subject: [PATCH] [turbofan] put TypeGuard into the effect chain and maintain it until EffectControlLinearizer We need to maintain TypeGuard nodes until the EffectControlLinearizer, because they can protect partial operations from floating above a check. In the linked bug, it was a DeadValue node that got scheduled too early. In LoadElimination and EscapeAnalysis, the inserted TypeGuard nodes might depend on map checks on the effect chain. Thus TypeGuard has to be an effect chain node too. Bug: chromium:800929 Change-Id: Icdcff96a2273d96b7f8cd6f85511ad62c1cb129a Reviewed-on: https://chromium-review.googlesource.com/860405 Reviewed-by: Jaroslav Sevcik Commit-Queue: Tobias Tebbi Cr-Commit-Position: refs/heads/master@{#50661} --- src/compiler/bytecode-graph-builder.cc | 2 + src/compiler/common-operator.cc | 2 +- src/compiler/effect-control-linearizer.cc | 12 +++-- src/compiler/escape-analysis-reducer.cc | 45 +++++++++++++------ src/compiler/escape-analysis-reducer.h | 2 +- src/compiler/graph-trimmer.cc | 1 + src/compiler/js-builtin-reducer.cc | 5 ++- src/compiler/js-call-reducer.cc | 32 ++++++------- .../js-native-context-specialization.cc | 4 +- src/compiler/js-typed-lowering.cc | 5 ++- src/compiler/loop-variable-optimizer.cc | 28 +++++++++--- src/compiler/loop-variable-optimizer.h | 7 ++- src/compiler/simplified-lowering.cc | 1 - 13 files changed, 94 insertions(+), 52 deletions(-) diff --git a/src/compiler/bytecode-graph-builder.cc b/src/compiler/bytecode-graph-builder.cc index 776fd0aede..54a924fce4 100644 --- a/src/compiler/bytecode-graph-builder.cc +++ b/src/compiler/bytecode-graph-builder.cc @@ -2671,7 +2671,9 @@ void BytecodeGraphBuilder::VisitForInNext() { // We need to rename the {index} here, as in case of OSR we loose the // information that the {index} is always a valid unsigned Smi value. index = graph()->NewNode(common()->TypeGuard(Type::UnsignedSmall()), index, + environment()->GetEffectDependency(), environment()->GetControlDependency()); + environment()->UpdateEffectDependency(index); FeedbackSlot slot = feedback_vector()->ToSlot(bytecode_iterator().GetIndexOperand(3)); diff --git a/src/compiler/common-operator.cc b/src/compiler/common-operator.cc index 09eb0e49fa..54af052d56 100644 --- a/src/compiler/common-operator.cc +++ b/src/compiler/common-operator.cc @@ -1136,7 +1136,7 @@ const Operator* CommonOperatorBuilder::TypeGuard(Type* type) { return new (zone()) Operator1( // -- IrOpcode::kTypeGuard, Operator::kPure, // opcode "TypeGuard", // name - 1, 0, 1, 1, 0, 0, // counts + 1, 1, 1, 1, 1, 0, // counts type); // parameter } diff --git a/src/compiler/effect-control-linearizer.cc b/src/compiler/effect-control-linearizer.cc index 62f87da3df..2b7d1ef9dd 100644 --- a/src/compiler/effect-control-linearizer.cc +++ b/src/compiler/effect-control-linearizer.cc @@ -145,9 +145,10 @@ bool HasIncomingBackEdges(BasicBlock* block) { return false; } -void RemoveRegionNode(Node* node) { +void RemoveRenameNode(Node* node) { DCHECK(IrOpcode::kFinishRegion == node->opcode() || - IrOpcode::kBeginRegion == node->opcode()); + IrOpcode::kBeginRegion == node->opcode() || + IrOpcode::kTypeGuard == node->opcode()); // Update the value/context uses to the value input of the finish node and // the effect uses to the effect input. for (Edge edge : node->use_edges()) { @@ -538,7 +539,7 @@ void EffectControlLinearizer::ProcessNode(Node* node, Node** frame_state, region_observability_ = RegionObservability::kObservable; // Update the value uses to the value input of the finish node and // the effect uses to the effect input. - return RemoveRegionNode(node); + return RemoveRenameNode(node); } if (node->opcode() == IrOpcode::kBeginRegion) { // Determine the observability for this region and use that for all @@ -548,7 +549,10 @@ void EffectControlLinearizer::ProcessNode(Node* node, Node** frame_state, region_observability_ = RegionObservabilityOf(node->op()); // Update the value uses to the value input of the finish node and // the effect uses to the effect input. - return RemoveRegionNode(node); + return RemoveRenameNode(node); + } + if (node->opcode() == IrOpcode::kTypeGuard) { + return RemoveRenameNode(node); } // Special treatment for checkpoint nodes. diff --git a/src/compiler/escape-analysis-reducer.cc b/src/compiler/escape-analysis-reducer.cc index aa2a1b2f3a..16a9d78faf 100644 --- a/src/compiler/escape-analysis-reducer.cc +++ b/src/compiler/escape-analysis-reducer.cc @@ -33,18 +33,39 @@ EscapeAnalysisReducer::EscapeAnalysisReducer( arguments_elements_(zone), zone_(zone) {} -Node* EscapeAnalysisReducer::MaybeGuard(Node* original, Node* replacement) { - // We might need to guard the replacement if the type of the {replacement} - // node is not in a sub-type relation to the type of the the {original} node. +Reduction EscapeAnalysisReducer::ReplaceNode(Node* original, + Node* replacement) { + const VirtualObject* vobject = + analysis_result().GetVirtualObject(replacement); + if (replacement->opcode() == IrOpcode::kDead || + (vobject && !vobject->HasEscaped())) { + RelaxEffectsAndControls(original); + return Replace(replacement); + } Type* const replacement_type = NodeProperties::GetType(replacement); Type* const original_type = NodeProperties::GetType(original); - if (!replacement_type->Is(original_type)) { - Node* const control = NodeProperties::GetControlInput(original); - replacement = jsgraph()->graph()->NewNode( - jsgraph()->common()->TypeGuard(original_type), replacement, control); - NodeProperties::SetType(replacement, original_type); + if (replacement_type->Is(original_type)) { + RelaxEffectsAndControls(original); + return Replace(replacement); } - return replacement; + + // We need to guard the replacement if we would widen the type otherwise. + DCHECK_EQ(1, original->op()->EffectOutputCount()); + DCHECK_EQ(1, original->op()->EffectInputCount()); + DCHECK_EQ(1, original->op()->ControlInputCount()); + Node* effect = NodeProperties::GetEffectInput(original); + Node* control = NodeProperties::GetControlInput(original); + original->TrimInputCount(0); + original->AppendInput(jsgraph()->zone(), replacement); + original->AppendInput(jsgraph()->zone(), effect); + original->AppendInput(jsgraph()->zone(), control); + NodeProperties::SetType( + original, + Type::Intersect(original_type, replacement_type, jsgraph()->zone())); + NodeProperties::ChangeOp(original, + jsgraph()->common()->TypeGuard(original_type)); + ReplaceWithValue(original, original, original, control); + return NoChange(); } namespace { @@ -74,11 +95,7 @@ Reduction EscapeAnalysisReducer::Reduce(Node* node) { DCHECK(node->opcode() != IrOpcode::kAllocate && node->opcode() != IrOpcode::kFinishRegion); DCHECK_NE(replacement, node); - if (replacement != jsgraph()->Dead()) { - replacement = MaybeGuard(node, replacement); - } - RelaxEffectsAndControls(node); - return Replace(replacement); + return ReplaceNode(node, replacement); } switch (node->opcode()) { diff --git a/src/compiler/escape-analysis-reducer.h b/src/compiler/escape-analysis-reducer.h index b89d4d03e8..29290d3a0a 100644 --- a/src/compiler/escape-analysis-reducer.h +++ b/src/compiler/escape-analysis-reducer.h @@ -97,7 +97,7 @@ class V8_EXPORT_PRIVATE EscapeAnalysisReducer final void ReduceFrameStateInputs(Node* node); Node* ReduceDeoptState(Node* node, Node* effect, Deduplicator* deduplicator); Node* ObjectIdNode(const VirtualObject* vobject); - Node* MaybeGuard(Node* original, Node* replacement); + Reduction ReplaceNode(Node* original, Node* replacement); JSGraph* jsgraph() const { return jsgraph_; } EscapeAnalysisResult analysis_result() const { return analysis_result_; } diff --git a/src/compiler/graph-trimmer.cc b/src/compiler/graph-trimmer.cc index 0d639c17f8..735a77db15 100644 --- a/src/compiler/graph-trimmer.cc +++ b/src/compiler/graph-trimmer.cc @@ -25,6 +25,7 @@ bool IsSideEffectFree(const Operator* op) { case IrOpcode::kLoad: case IrOpcode::kLoadElement: case IrOpcode::kLoadField: + case IrOpcode::kTypeGuard: return true; default: return false; diff --git a/src/compiler/js-builtin-reducer.cc b/src/compiler/js-builtin-reducer.cc index 18cfac9866..7ff2bf6d5e 100644 --- a/src/compiler/js-builtin-reducer.cc +++ b/src/compiler/js-builtin-reducer.cc @@ -1139,8 +1139,9 @@ Reduction JSBuiltinReducer::ReduceCollectionIteratorNext( // Abort loop with resulting value. Node* control = graph()->NewNode(common()->IfFalse(), branch1); Node* effect = etrue0; - Node* value = graph()->NewNode( - common()->TypeGuard(Type::NonInternal()), entry_key, control); + Node* value = effect = + graph()->NewNode(common()->TypeGuard(Type::NonInternal()), + entry_key, effect, control); Node* done = jsgraph()->FalseConstant(); // Advance the index on the {receiver}. diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index e0fb1943e1..1f8e7a2cef 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -950,8 +950,8 @@ Reduction JSCallReducer::ReduceArrayForEach(Handle function, // The contract is that we don't leak "the hole" into "user JavaScript", // so we must rename the {element} here to explicitly exclude "the hole" // from the type of {element}. - element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()), - element, control); + element = effect = graph()->NewNode( + common()->TypeGuard(Type::NonInternal()), element, effect, control); } frame_state = CreateJavaScriptBuiltinContinuationFrameState( @@ -1164,8 +1164,8 @@ Reduction JSCallReducer::ReduceArrayReduce(Handle function, // The contract is that we don't leak "the hole" into "user JavaScript", // so we must rename the {element} here to explicitly exclude "the hole" // from the type of {element}. - element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()), - element, control); + element = effect = graph()->NewNode( + common()->TypeGuard(Type::NonInternal()), element, effect, control); } frame_state = CreateJavaScriptBuiltinContinuationFrameState( @@ -1401,8 +1401,8 @@ Reduction JSCallReducer::ReduceArrayReduceRight(Handle function, // The contract is that we don't leak "the hole" into "user JavaScript", // so we must rename the {element} here to explicitly exclude "the hole" // from the type of {element}. - element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()), - element, control); + element = effect = graph()->NewNode( + common()->TypeGuard(Type::NonInternal()), element, effect, control); } frame_state = CreateJavaScriptBuiltinContinuationFrameState( @@ -1612,8 +1612,8 @@ Reduction JSCallReducer::ReduceArrayMap(Handle function, // The contract is that we don't leak "the hole" into "user JavaScript", // so we must rename the {element} here to explicitly exclude "the hole" // from the type of {element}. - element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()), - element, control); + element = effect = graph()->NewNode( + common()->TypeGuard(Type::NonInternal()), element, effect, control); } // This frame state is dealt with by hand in @@ -1838,8 +1838,8 @@ Reduction JSCallReducer::ReduceArrayFilter(Handle function, // The contract is that we don't leak "the hole" into "user JavaScript", // so we must rename the {element} here to explicitly exclude "the hole" // from the type of {element}. - element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()), - element, control); + element = effect = graph()->NewNode( + common()->TypeGuard(Type::NonInternal()), element, effect, control); } Node* callback_value = nullptr; @@ -2160,8 +2160,8 @@ Node* JSCallReducer::DoFilterPostCallbackWork(ElementsKind kind, Node** control, // We know that {to} is in Unsigned31 range here, being smaller than // {original_length} at all times. - Node* checked_to = - graph()->NewNode(common()->TypeGuard(Type::Unsigned31()), to, if_true); + Node* checked_to = etrue = graph()->NewNode( + common()->TypeGuard(Type::Unsigned31()), to, etrue, if_true); Node* elements_length = etrue = graph()->NewNode( simplified()->LoadField(AccessBuilder::ForFixedArrayLength()), elements, etrue, if_true); @@ -2405,8 +2405,8 @@ Reduction JSCallReducer::ReduceArrayEvery(Handle function, // The contract is that we don't leak "the hole" into "user JavaScript", // so we must rename the {element} here to explicitly exclude "the hole" // from the type of {element}. - element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()), - element, control); + element = effect = graph()->NewNode( + common()->TypeGuard(Type::NonInternal()), element, effect, control); } Node* callback_value = nullptr; @@ -2632,8 +2632,8 @@ Reduction JSCallReducer::ReduceArraySome(Handle function, // The contract is that we don't leak "the hole" into "user JavaScript", // so we must rename the {element} here to explicitly exclude "the hole" // from the type of {element}. - element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()), - element, control); + element = effect = graph()->NewNode( + common()->TypeGuard(Type::NonInternal()), element, effect, control); } Node* callback_value = nullptr; diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc index 0de0190206..b2f8c567e2 100644 --- a/src/compiler/js-native-context-specialization.cc +++ b/src/compiler/js-native-context-specialization.cc @@ -2584,8 +2584,8 @@ Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore( common()->Select(MachineRepresentation::kTaggedSigned), graph()->NewNode(simplified()->ObjectIsSmi(), properties), properties, jsgraph()->SmiConstant(PropertyArray::kNoHashSentinel)); - hash = graph()->NewNode(common()->TypeGuard(Type::SignedSmall()), hash, - control); + hash = effect = graph()->NewNode(common()->TypeGuard(Type::SignedSmall()), + hash, effect, control); hash = graph()->NewNode(simplified()->NumberShiftLeft(), hash, jsgraph()->Constant(PropertyArray::HashField::kShift)); diff --git a/src/compiler/js-typed-lowering.cc b/src/compiler/js-typed-lowering.cc index 7bc3b6081d..c265caf9f0 100644 --- a/src/compiler/js-typed-lowering.cc +++ b/src/compiler/js-typed-lowering.cc @@ -707,8 +707,9 @@ Reduction JSTypedLowering::ReduceCreateConsString(Node* node) { Revisit(graph()->end()); } control = graph()->NewNode(common()->IfTrue(), branch); - length = graph()->NewNode( - common()->TypeGuard(type_cache_.kStringLengthType), length, control); + length = effect = + graph()->NewNode(common()->TypeGuard(type_cache_.kStringLengthType), + length, effect, control); } Node* value = diff --git a/src/compiler/loop-variable-optimizer.cc b/src/compiler/loop-variable-optimizer.cc index b291030534..1e93de5124 100644 --- a/src/compiler/loop-variable-optimizer.cc +++ b/src/compiler/loop-variable-optimizer.cc @@ -301,7 +301,8 @@ const InductionVariable* LoopVariableOptimizer::FindInductionVariable( InductionVariable* LoopVariableOptimizer::TryGetInductionVariable(Node* phi) { DCHECK_EQ(2, phi->op()->ValueInputCount()); - DCHECK_EQ(IrOpcode::kLoop, NodeProperties::GetControlInput(phi)->opcode()); + Node* loop = NodeProperties::GetControlInput(phi); + DCHECK_EQ(IrOpcode::kLoop, loop->opcode()); Node* initial = phi->InputAt(0); Node* arith = phi->InputAt(1); InductionVariable::ArithmeticType arithmeticType; @@ -320,9 +321,18 @@ InductionVariable* LoopVariableOptimizer::TryGetInductionVariable(Node* phi) { // TODO(jarin) Support both sides. if (arith->InputAt(0) != phi) return nullptr; + Node* effect_phi = nullptr; + for (Node* use : loop->uses()) { + if (use->opcode() == IrOpcode::kEffectPhi) { + DCHECK_NULL(effect_phi); + effect_phi = use; + } + } + if (!effect_phi) return nullptr; + Node* incr = arith->InputAt(1); - return new (zone()) - InductionVariable(phi, arith, incr, initial, zone(), arithmeticType); + return new (zone()) InductionVariable(phi, effect_phi, arith, incr, initial, + zone(), arithmeticType); } void LoopVariableOptimizer::DetectInductionVariables(Node* loop) { @@ -392,10 +402,14 @@ void LoopVariableOptimizer::ChangeToPhisAndInsertGuards() { Type* backedge_type = NodeProperties::GetType(backedge_value); Type* phi_type = NodeProperties::GetType(induction_var->phi()); if (!backedge_type->Is(phi_type)) { - Node* backedge_control = - NodeProperties::GetControlInput(induction_var->phi())->InputAt(1); - Node* rename = graph()->NewNode(common()->TypeGuard(phi_type), - backedge_value, backedge_control); + Node* loop = NodeProperties::GetControlInput(induction_var->phi()); + Node* backedge_control = loop->InputAt(1); + Node* backedge_effect = + NodeProperties::GetEffectInput(induction_var->effect_phi(), 1); + Node* rename = + graph()->NewNode(common()->TypeGuard(phi_type), backedge_value, + backedge_effect, backedge_control); + induction_var->effect_phi()->ReplaceInput(1, rename); induction_var->phi()->ReplaceInput(1, rename); } } diff --git a/src/compiler/loop-variable-optimizer.h b/src/compiler/loop-variable-optimizer.h index 8054ec16c8..9eec614070 100644 --- a/src/compiler/loop-variable-optimizer.h +++ b/src/compiler/loop-variable-optimizer.h @@ -18,6 +18,7 @@ class Node; class InductionVariable : public ZoneObject { public: Node* phi() const { return phi_; } + Node* effect_phi() const { return effect_phi_; } Node* arith() const { return arith_; } Node* increment() const { return increment_; } Node* init_value() const { return init_value_; } @@ -39,9 +40,10 @@ class InductionVariable : public ZoneObject { private: friend class LoopVariableOptimizer; - InductionVariable(Node* phi, Node* arith, Node* increment, Node* init_value, - Zone* zone, ArithmeticType arithmeticType) + InductionVariable(Node* phi, Node* effect_phi, Node* arith, Node* increment, + Node* init_value, Zone* zone, ArithmeticType arithmeticType) : phi_(phi), + effect_phi_(effect_phi), arith_(arith), increment_(increment), init_value_(init_value), @@ -53,6 +55,7 @@ class InductionVariable : public ZoneObject { void AddLowerBound(Node* bound, ConstraintKind kind); Node* phi_; + Node* effect_phi_; Node* arith_; Node* increment_; Node* init_value_; diff --git a/src/compiler/simplified-lowering.cc b/src/compiler/simplified-lowering.cc index c59ae529ed..36c45d4194 100644 --- a/src/compiler/simplified-lowering.cc +++ b/src/compiler/simplified-lowering.cc @@ -2977,7 +2977,6 @@ class RepresentationSelector { } ProcessRemainingInputs(node, 1); SetOutput(node, representation); - if (lower()) DeferReplacement(node, node->InputAt(0)); return; }