From 7c9cf0b3a117c6670372a40f623e037ace23537d Mon Sep 17 00:00:00 2001 From: "fschneider@chromium.org" Date: Mon, 6 Jun 2011 14:57:25 +0000 Subject: [PATCH] Re-land r8140: Deoptimize on never-executed code-paths. Original cl: http://codereview.chromium.org/7105015 I'm removing the test GlobalLoadICGC test that was introduced for testing inlined global cell loads (in the classic backend) and has an invalid assumption about the number of global objects referenced from a v8 context. We don't have this feature with Crankshaft anymore. Review URL: http://codereview.chromium.org/7112032 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@8185 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/lithium-arm.cc | 5 ++ src/hydrogen-instructions.cc | 10 +++ src/hydrogen-instructions.h | 15 +++++ src/hydrogen.cc | 124 ++++++++++++++++++++++------------- src/hydrogen.h | 4 -- src/ia32/lithium-ia32.cc | 5 ++ src/type-info.cc | 6 +- src/x64/lithium-x64.cc | 5 ++ test/cctest/test-api.cc | 42 ------------ test/mjsunit/regress/regress-1118.js | 2 - 10 files changed, 121 insertions(+), 97 deletions(-) diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc index b9b62cf..1f2b2f6 100644 --- a/src/arm/lithium-arm.cc +++ b/src/arm/lithium-arm.cc @@ -808,6 +808,11 @@ LInstruction* LChunkBuilder::DoBlockEntry(HBlockEntry* instr) { } +LInstruction* LChunkBuilder::DoSoftDeoptimize(HSoftDeoptimize* instr) { + return AssignEnvironment(new LDeoptimize); +} + + LInstruction* LChunkBuilder::DoDeoptimize(HDeoptimize* instr) { return AssignEnvironment(new LDeoptimize); } diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index 545f481..cc2dcf5 100644 --- a/src/hydrogen-instructions.cc +++ b/src/hydrogen-instructions.cc @@ -1083,6 +1083,16 @@ void HSimulate::PrintDataTo(StringStream* stream) { } +void HDeoptimize::PrintDataTo(StringStream* stream) { + if (OperandCount() == 0) return; + OperandAt(0)->PrintNameTo(stream); + for (int i = 1; i < OperandCount(); ++i) { + stream->Add(" "); + OperandAt(i)->PrintNameTo(stream); + } +} + + void HEnterInlined::PrintDataTo(StringStream* stream) { SmartPointer name = function()->debug_name()->ToCString(); stream->Add("%s, id=%d", *name, function()->id()); diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 15e3e1e..e8d8e81 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -146,6 +146,7 @@ class LChunkBuilder; V(Shl) \ V(Shr) \ V(Simulate) \ + V(SoftDeoptimize) \ V(StackCheck) \ V(StoreContextSlot) \ V(StoreGlobalCell) \ @@ -847,6 +848,19 @@ class HBlockEntry: public HTemplateInstruction<0> { }; +// We insert soft-deoptimize when we hit code with unknown typefeedback, +// so that we get a chance of re-optimizing with useful typefeedback. +// HSoftDeoptimize does not end a basic block as opposed to HDeoptimize. +class HSoftDeoptimize: public HTemplateInstruction<0> { + public: + virtual Representation RequiredInputRepresentation(int index) const { + return Representation::None(); + } + + DECLARE_CONCRETE_INSTRUCTION(SoftDeoptimize) +}; + + class HDeoptimize: public HControlInstruction { public: explicit HDeoptimize(int environment_length) @@ -859,6 +873,7 @@ class HDeoptimize: public HControlInstruction { virtual int OperandCount() { return values_.length(); } virtual HValue* OperandAt(int index) { return values_[index]; } + virtual void PrintDataTo(StringStream* stream); void AddEnvironmentValue(HValue* value) { values_.Add(NULL); diff --git a/src/hydrogen.cc b/src/hydrogen.cc index c4ac6c1..3ce4a5b 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -68,8 +68,7 @@ HBasicBlock::HBasicBlock(HGraph* graph) last_instruction_index_(-1), deleted_phis_(4), parent_loop_header_(NULL), - is_inline_return_target_(false) { -} + is_inline_return_target_(false) { } void HBasicBlock::AttachLoopInformation() { @@ -4310,21 +4309,22 @@ bool HGraphBuilder::TryInline(Call* expr) { if (inlined_test_context() != NULL) { HBasicBlock* if_true = inlined_test_context()->if_true(); HBasicBlock* if_false = inlined_test_context()->if_false(); - if_true->SetJoinId(expr->id()); - if_false->SetJoinId(expr->id()); - ASSERT(ast_context() == inlined_test_context()); + // Pop the return test context from the expression context stack. + ASSERT(ast_context() == inlined_test_context()); ClearInlinedTestContext(); // Forward to the real test context. - HBasicBlock* true_target = TestContext::cast(ast_context())->if_true(); - HBasicBlock* false_target = TestContext::cast(ast_context())->if_false(); - if_true->Goto(true_target, false); - if_false->Goto(false_target, false); - - // TODO(kmillikin): Come up with a better way to handle this. It is too - // subtle. NULL here indicates that the enclosing context has no control - // flow to handle. + if (if_true->HasPredecessor()) { + if_true->SetJoinId(expr->id()); + HBasicBlock* true_target = TestContext::cast(ast_context())->if_true(); + if_true->Goto(true_target, false); + } + if (if_false->HasPredecessor()) { + if_false->SetJoinId(expr->id()); + HBasicBlock* false_target = TestContext::cast(ast_context())->if_false(); + if_false->Goto(false_target, false); + } set_current_block(NULL); } else if (function_return()->HasPredecessor()) { @@ -4791,6 +4791,10 @@ void HGraphBuilder::VisitSub(UnaryOperation* expr) { HValue* value = Pop(); HInstruction* instr = new(zone()) HMul(value, graph_->GetConstantMinus1()); TypeInfo info = oracle()->UnaryType(expr); + if (info.IsUninitialized()) { + AddInstruction(new(zone()) HSoftDeoptimize); + info = TypeInfo::Unknown(); + } Representation rep = ToRepresentation(info); TraceRepresentation(expr->op(), info, instr, rep); instr->AssumeRepresentation(rep); @@ -4801,6 +4805,10 @@ void HGraphBuilder::VisitSub(UnaryOperation* expr) { void HGraphBuilder::VisitBitNot(UnaryOperation* expr) { CHECK_ALIVE(VisitForValue(expr->expression())); HValue* value = Pop(); + TypeInfo info = oracle()->UnaryType(expr); + if (info.IsUninitialized()) { + AddInstruction(new(zone()) HSoftDeoptimize); + } HInstruction* instr = new(zone()) HBitNot(value); ast_context()->ReturnInstruction(instr, expr->id()); } @@ -5032,7 +5040,57 @@ HInstruction* HGraphBuilder::BuildBinaryOperation(BinaryOperation* expr, HValue* left, HValue* right) { TypeInfo info = oracle()->BinaryType(expr); - HInstruction* instr = BuildBinaryOperation(expr->op(), left, right, info); + if (info.IsUninitialized()) { + AddInstruction(new(zone()) HSoftDeoptimize); + info = TypeInfo::Unknown(); + } + HInstruction* instr = NULL; + switch (expr->op()) { + case Token::ADD: + if (info.IsString()) { + AddInstruction(new(zone()) HCheckNonSmi(left)); + AddInstruction(HCheckInstanceType::NewIsString(left)); + AddInstruction(new(zone()) HCheckNonSmi(right)); + AddInstruction(HCheckInstanceType::NewIsString(right)); + instr = new(zone()) HStringAdd(left, right); + } else { + instr = new(zone()) HAdd(left, right); + } + break; + case Token::SUB: + instr = new(zone()) HSub(left, right); + break; + case Token::MUL: + instr = new(zone()) HMul(left, right); + break; + case Token::MOD: + instr = new(zone()) HMod(left, right); + break; + case Token::DIV: + instr = new(zone()) HDiv(left, right); + break; + case Token::BIT_XOR: + instr = new(zone()) HBitXor(left, right); + break; + case Token::BIT_AND: + instr = new(zone()) HBitAnd(left, right); + break; + case Token::BIT_OR: + instr = new(zone()) HBitOr(left, right); + break; + case Token::SAR: + instr = new(zone()) HSar(left, right); + break; + case Token::SHR: + instr = new(zone()) HShr(left, right); + break; + case Token::SHL: + instr = new(zone()) HShl(left, right); + break; + default: + UNREACHABLE(); + } + // If we hit an uninitialized binary op stub we will get type info // for a smi operation. If one of the operands is a constant string // do not generate code assuming it is a smi operation. @@ -5052,36 +5110,6 @@ HInstruction* HGraphBuilder::BuildBinaryOperation(BinaryOperation* expr, } -HInstruction* HGraphBuilder::BuildBinaryOperation( - Token::Value op, HValue* left, HValue* right, TypeInfo info) { - switch (op) { - case Token::ADD: - if (info.IsString()) { - AddInstruction(new(zone()) HCheckNonSmi(left)); - AddInstruction(HCheckInstanceType::NewIsString(left)); - AddInstruction(new(zone()) HCheckNonSmi(right)); - AddInstruction(HCheckInstanceType::NewIsString(right)); - return new(zone()) HStringAdd(left, right); - } else { - return new(zone()) HAdd(left, right); - } - case Token::SUB: return new(zone()) HSub(left, right); - case Token::MUL: return new(zone()) HMul(left, right); - case Token::MOD: return new(zone()) HMod(left, right); - case Token::DIV: return new(zone()) HDiv(left, right); - case Token::BIT_XOR: return new(zone()) HBitXor(left, right); - case Token::BIT_AND: return new(zone()) HBitAnd(left, right); - case Token::BIT_OR: return new(zone()) HBitOr(left, right); - case Token::SAR: return new(zone()) HSar(left, right); - case Token::SHR: return new(zone()) HShr(left, right); - case Token::SHL: return new(zone()) HShl(left, right); - default: - UNREACHABLE(); - return NULL; - } -} - - // Check for the form (%_ClassOf(foo) === 'BarClass'). static bool IsClassOfTest(CompareOperation* expr) { if (expr->op() != Token::EQ_STRICT) return false; @@ -5279,6 +5307,13 @@ void HGraphBuilder::VisitCompareOperation(CompareOperation* expr) { return; } + TypeInfo type_info = oracle()->CompareType(expr); + // Check if this expression was ever executed according to type feedback. + if (type_info.IsUninitialized()) { + AddInstruction(new(zone()) HSoftDeoptimize); + type_info = TypeInfo::Unknown(); + } + CHECK_ALIVE(VisitForValue(expr->left())); CHECK_ALIVE(VisitForValue(expr->right())); @@ -5286,7 +5321,6 @@ void HGraphBuilder::VisitCompareOperation(CompareOperation* expr) { HValue* left = Pop(); Token::Value op = expr->op(); - TypeInfo type_info = oracle()->CompareType(expr); HInstruction* instr = NULL; if (op == Token::INSTANCEOF) { // Check to see if the rhs of the instanceof is a global function not diff --git a/src/hydrogen.h b/src/hydrogen.h index 6034801..3be8fd8 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -878,10 +878,6 @@ class HGraphBuilder: public AstVisitor { HInstruction* BuildBinaryOperation(BinaryOperation* expr, HValue* left, HValue* right); - HInstruction* BuildBinaryOperation(Token::Value op, - HValue* left, - HValue* right, - TypeInfo info); HInstruction* BuildIncrement(bool returns_original_input, CountOperation* expr); HLoadNamedField* BuildLoadNamedField(HValue* object, diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index 2c10b4d..fc4d71e 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -804,6 +804,11 @@ LInstruction* LChunkBuilder::DoBlockEntry(HBlockEntry* instr) { } +LInstruction* LChunkBuilder::DoSoftDeoptimize(HSoftDeoptimize* instr) { + return AssignEnvironment(new LDeoptimize); +} + + LInstruction* LChunkBuilder::DoDeoptimize(HDeoptimize* instr) { return AssignEnvironment(new LDeoptimize); } diff --git a/src/type-info.cc b/src/type-info.cc index 5f794bd..37ab706 100644 --- a/src/type-info.cc +++ b/src/type-info.cc @@ -224,8 +224,7 @@ TypeInfo TypeFeedbackOracle::CompareType(CompareOperation* expr) { switch (state) { case CompareIC::UNINITIALIZED: // Uninitialized means never executed. - // TODO(fschneider): Introduce a separate value for never-executed ICs. - return unknown; + return TypeInfo::Uninitialized(); case CompareIC::SMIS: return TypeInfo::Smi(); case CompareIC::HEAP_NUMBERS: @@ -286,8 +285,7 @@ TypeInfo TypeFeedbackOracle::BinaryType(BinaryOperation* expr) { switch (type) { case BinaryOpIC::UNINITIALIZED: // Uninitialized means never executed. - // TODO(fschneider): Introduce a separate value for never-executed ICs - return unknown; + return TypeInfo::Uninitialized(); case BinaryOpIC::SMI: switch (result_type) { case BinaryOpIC::UNINITIALIZED: diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc index d84d024..12325e5 100644 --- a/src/x64/lithium-x64.cc +++ b/src/x64/lithium-x64.cc @@ -803,6 +803,11 @@ LInstruction* LChunkBuilder::DoBlockEntry(HBlockEntry* instr) { } +LInstruction* LChunkBuilder::DoSoftDeoptimize(HSoftDeoptimize* instr) { + return AssignEnvironment(new LDeoptimize); +} + + LInstruction* LChunkBuilder::DoDeoptimize(HDeoptimize* instr) { return AssignEnvironment(new LDeoptimize); } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 34d70b6..ca20baf 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -14038,48 +14038,6 @@ TEST(DontDeleteCellLoadICAPI) { } -TEST(GlobalLoadICGC) { - const char* function_code = - "function readCell() { while (true) { return cell; } }"; - - // Check inline load code for a don't delete cell is cleared during - // GC. - { - v8::HandleScope scope; - LocalContext context; - CompileRun("var cell = \"value\";"); - ExpectBoolean("delete cell", false); - CompileRun(function_code); - ExpectString("readCell()", "value"); - ExpectString("readCell()", "value"); - } - { - v8::HandleScope scope; - LocalContext context2; - // Hold the code object in the second context. - CompileRun(function_code); - CheckSurvivingGlobalObjectsCount(1); - } - - // Check inline load code for a deletable cell is cleared during GC. - { - v8::HandleScope scope; - LocalContext context; - CompileRun("cell = \"value\";"); - CompileRun(function_code); - ExpectString("readCell()", "value"); - ExpectString("readCell()", "value"); - } - { - v8::HandleScope scope; - LocalContext context2; - // Hold the code object in the second context. - CompileRun(function_code); - CheckSurvivingGlobalObjectsCount(1); - } -} - - TEST(RegExp) { v8::HandleScope scope; LocalContext context; diff --git a/test/mjsunit/regress/regress-1118.js b/test/mjsunit/regress/regress-1118.js index 86b8981..7e0461d 100644 --- a/test/mjsunit/regress/regress-1118.js +++ b/test/mjsunit/regress/regress-1118.js @@ -55,8 +55,6 @@ function h() { while (%GetOptimizationStatus(h) == 2) { for (var j = 0; j < 100; j++) g(); } - assertTrue(%GetOptimizationStatus(h) == 1 || - %GetOptimizationStatus(h) == 3); g(); } } -- 2.7.4