From e48ec1c4bd25c018e7cb7363b10230be7d4d3a85 Mon Sep 17 00:00:00 2001 From: "svenpanne@chromium.org" Date: Thu, 28 Jul 2011 13:33:51 +0000 Subject: [PATCH] Use type info for the ToBoolean translation in crankshaft. To do this, the Branch instruction needs to carry around a temporary register, but only when the crankshafted code will make a map access. When the crankshafted code sees an object of a type it hasn't encountered before, it will always trigger a deopt. Another option in theses cases would be calling a ToBooleanStub which can handle all types, but then one has to be careful to *not* trigger a GC (which is currently a bit tricky to achieve). Const-corrected ToBoolean::Types. Moved the NeedsMap logic into ToBoolean::Types itself, where it belongs. This patch improves a lot of benchmarks, crypto-orig even by 16.7%, but slows down others. The slowdown has to be investigated, but I'd like to get this patch out first to fix the flakiness problems we currently have due to the previous crankshafted ToBoolean. Review URL: http://codereview.chromium.org/7461107 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@8758 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/code-stubs.cc | 12 ++- src/code-stubs.h | 5 +- src/ia32/code-stubs-ia32.cc | 10 +-- src/ia32/lithium-codegen-ia32.cc | 148 ++++++++++++++++++++++++------- src/ia32/lithium-ia32.cc | 5 +- src/ia32/lithium-ia32.h | 5 +- 6 files changed, 140 insertions(+), 45 deletions(-) diff --git a/src/code-stubs.cc b/src/code-stubs.cc index cc06115230..0cba275c3e 100644 --- a/src/code-stubs.cc +++ b/src/code-stubs.cc @@ -336,7 +336,7 @@ void ToBooleanStub::PrintName(StringStream* stream) { } -void ToBooleanStub::Types::Print(StringStream* stream) { +void ToBooleanStub::Types::Print(StringStream* stream) const { if (IsEmpty()) stream->Add("None"); if (Contains(UNDEFINED)) stream->Add("Undefined"); if (Contains(BOOLEAN)) stream->Add("Bool"); @@ -349,7 +349,7 @@ void ToBooleanStub::Types::Print(StringStream* stream) { } -void ToBooleanStub::Types::TraceTransition(Types to) { +void ToBooleanStub::Types::TraceTransition(Types to) const { if (!FLAG_trace_ic) return; char buffer[100]; NoAllocationStringAllocator allocator(buffer, @@ -395,4 +395,12 @@ bool ToBooleanStub::Types::Record(Handle object) { } +bool ToBooleanStub::Types::NeedsMap() const { + return Contains(ToBooleanStub::SPEC_OBJECT) + || Contains(ToBooleanStub::STRING) + || Contains(ToBooleanStub::HEAP_NUMBER) + || Contains(ToBooleanStub::INTERNAL_OBJECT); +} + + } } // namespace v8::internal diff --git a/src/code-stubs.h b/src/code-stubs.h index 6527fde18a..b9e49c8e71 100644 --- a/src/code-stubs.h +++ b/src/code-stubs.h @@ -926,9 +926,10 @@ class ToBooleanStub: public CodeStub { bool Contains(Type type) const { return set_.Contains(type); } void Add(Type type) { set_.Add(type); } byte ToByte() const { return set_.ToIntegral(); } - void Print(StringStream* stream); - void TraceTransition(Types to); + void Print(StringStream* stream) const; + void TraceTransition(Types to) const; bool Record(Handle object); + bool NeedsMap() const; private: EnumSet set_; diff --git a/src/ia32/code-stubs-ia32.cc b/src/ia32/code-stubs-ia32.cc index cd2601ee52..598596bd76 100644 --- a/src/ia32/code-stubs-ia32.cc +++ b/src/ia32/code-stubs-ia32.cc @@ -258,12 +258,6 @@ void ToBooleanStub::Generate(MacroAssembler* masm) { // 'null' -> false. CheckOddball(masm, NULL_TYPE, factory->null_value(), false, &patch); - bool need_map = - types_.Contains(SPEC_OBJECT) | - types_.Contains(STRING) | - types_.Contains(HEAP_NUMBER) | - types_.Contains(INTERNAL_OBJECT); - if (types_.Contains(SMI)) { // Smis: 0 -> false, all other -> true Label not_smi; @@ -274,12 +268,12 @@ void ToBooleanStub::Generate(MacroAssembler* masm) { } __ ret(1 * kPointerSize); __ bind(¬_smi); - } else if (need_map) { + } else if (types_.NeedsMap()) { // If we need a map later and have a Smi -> patch. __ JumpIfSmi(argument, &patch, Label::kNear); } - if (need_map) { + if (types_.NeedsMap()) { __ mov(map, FieldOperand(argument, HeapObject::kMapOffset)); // Everything with a map could be undetectable, so check this now. diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 6f99781c62..22135e8a76 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -1392,45 +1392,133 @@ void LCodeGen::DoBranch(LBranch* instr) { EmitBranch(true_block, false_block, not_equal); } else { ASSERT(r.IsTagged()); + Factory* factory = this->factory(); Register reg = ToRegister(instr->InputAt(0)); if (instr->hydrogen()->value()->type().IsBoolean()) { - __ cmp(reg, factory()->true_value()); + __ cmp(reg, factory->true_value()); EmitBranch(true_block, false_block, equal); } else { Label* true_label = chunk_->GetAssemblyLabel(true_block); Label* false_label = chunk_->GetAssemblyLabel(false_block); - __ cmp(reg, factory()->undefined_value()); - __ j(equal, false_label); - __ cmp(reg, factory()->true_value()); - __ j(equal, true_label); - __ cmp(reg, factory()->false_value()); - __ j(equal, false_label); - __ test(reg, Operand(reg)); - __ j(equal, false_label); - __ JumpIfSmi(reg, true_label); + ToBooleanStub::Types expected = instr->hydrogen()->expected_input_types(); + // Avoid deopts in the case where we've never executed this path before. + if (expected.IsEmpty()) expected = ToBooleanStub::all_types(); - // Test for double values. Zero is false. - Label call_stub; - __ cmp(FieldOperand(reg, HeapObject::kMapOffset), - factory()->heap_number_map()); - __ j(not_equal, &call_stub, Label::kNear); - __ fldz(); - __ fld_d(FieldOperand(reg, HeapNumber::kValueOffset)); - __ FCmp(); - __ j(zero, false_label); - __ jmp(true_label); + if (expected.Contains(ToBooleanStub::UNDEFINED)) { + // undefined -> false. + __ cmp(reg, factory->undefined_value()); + __ j(equal, false_label); + } else if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // We've seen undefined for the first time -> deopt. + __ cmp(reg, factory->undefined_value()); + DeoptimizeIf(equal, instr->environment()); + } - // The conversion stub doesn't cause garbage collections so it's - // safe to not record a safepoint after the call. - __ bind(&call_stub); - ToBooleanStub stub(eax, ToBooleanStub::all_types()); - __ pushad(); - __ push(reg); - __ CallStub(&stub); - __ test(eax, Operand(eax)); - __ popad(); - EmitBranch(true_block, false_block, not_zero); + if (expected.Contains(ToBooleanStub::BOOLEAN)) { + // true -> true. + __ cmp(reg, factory->true_value()); + __ j(equal, true_label); + } else if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // We've seen a string for the first time -> deopt. + __ cmp(reg, factory->true_value()); + DeoptimizeIf(equal, instr->environment()); + } + + if (expected.Contains(ToBooleanStub::BOOLEAN)) { + // false -> false. + __ cmp(reg, factory->false_value()); + __ j(equal, false_label); + } else if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // We've seen a string for the first time -> deopt. + __ cmp(reg, factory->false_value()); + DeoptimizeIf(equal, instr->environment()); + } + + if (expected.Contains(ToBooleanStub::NULL_TYPE)) { + // 'null' -> false. + __ cmp(reg, factory->null_value()); + __ j(equal, false_label); + } else if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // We've seen null for the first time -> deopt. + __ cmp(reg, factory->null_value()); + DeoptimizeIf(equal, instr->environment()); + } + + if (expected.Contains(ToBooleanStub::SMI)) { + // Smis: 0 -> false, all other -> true. + __ test(reg, Operand(reg)); + __ j(equal, false_label); + __ JumpIfSmi(reg, true_label); + } else if (expected.NeedsMap()) { + // If we need a map later and have a Smi -> deopt. + __ test(reg, Immediate(kSmiTagMask)); + DeoptimizeIf(zero, instr->environment()); + } + + Register map; + if (expected.NeedsMap()) { + map = ToRegister(instr->TempAt(0)); + ASSERT(!map.is(reg)); + __ mov(map, FieldOperand(reg, HeapObject::kMapOffset)); + // Everything with a map could be undetectable, so check this now. + __ test_b(FieldOperand(map, Map::kBitFieldOffset), + 1 << Map::kIsUndetectable); + // Undetectable -> false. + __ j(not_zero, false_label); + } + + if (expected.Contains(ToBooleanStub::SPEC_OBJECT)) { + // spec object -> true. + __ CmpInstanceType(map, FIRST_SPEC_OBJECT_TYPE); + __ j(above_equal, true_label); + } else if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // We've seen a spec object for the first time -> deopt. + __ CmpInstanceType(map, FIRST_SPEC_OBJECT_TYPE); + DeoptimizeIf(above_equal, instr->environment()); + } + + if (expected.Contains(ToBooleanStub::STRING)) { + // String value -> false iff empty. + Label not_string; + __ CmpInstanceType(map, FIRST_NONSTRING_TYPE); + __ j(above_equal, ¬_string, Label::kNear); + __ cmp(FieldOperand(reg, String::kLengthOffset), Immediate(0)); + __ j(not_zero, true_label); + __ jmp(false_label); + __ bind(¬_string); + } else if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // We've seen a string for the first time -> deopt + __ CmpInstanceType(map, FIRST_NONSTRING_TYPE); + DeoptimizeIf(below, instr->environment()); + } + + if (expected.Contains(ToBooleanStub::HEAP_NUMBER)) { + // heap number -> false iff +0, -0, or NaN. + Label not_heap_number; + __ cmp(FieldOperand(reg, HeapObject::kMapOffset), + factory->heap_number_map()); + __ j(not_equal, ¬_heap_number, Label::kNear); + __ fldz(); + __ fld_d(FieldOperand(reg, HeapNumber::kValueOffset)); + __ FCmp(); + __ j(zero, false_label); + __ jmp(true_label); + __ bind(¬_heap_number); + } else if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // We've seen a heap number for the first time -> deopt. + __ cmp(FieldOperand(reg, HeapObject::kMapOffset), + factory->heap_number_map()); + DeoptimizeIf(equal, instr->environment()); + } + + if (expected.Contains(ToBooleanStub::INTERNAL_OBJECT)) { + // internal objects -> true + __ jmp(true_label); + } else { + // We've seen something for the first time -> deopt. + DeoptimizeIf(no_condition, instr->environment()); + } } } } diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index 9951d2540b..fbeb42b358 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -1041,7 +1041,10 @@ LInstruction* LChunkBuilder::DoBranch(HBranch* instr) { : instr->SecondSuccessor(); return new LGoto(successor->block_id()); } - return new LBranch(UseRegisterAtStart(v)); + ToBooleanStub::Types expected = instr->expected_input_types(); + bool needs_temp = expected.NeedsMap() || expected.IsEmpty(); + LOperand* temp = needs_temp ? TempRegister() : NULL; + return AssignEnvironment(new LBranch(UseRegister(v), temp)); } diff --git a/src/ia32/lithium-ia32.h b/src/ia32/lithium-ia32.h index 6b60a6e40e..efa048dd24 100644 --- a/src/ia32/lithium-ia32.h +++ b/src/ia32/lithium-ia32.h @@ -876,10 +876,11 @@ class LConstantT: public LTemplateInstruction<1, 0, 0> { }; -class LBranch: public LControlInstruction<1, 0> { +class LBranch: public LControlInstruction<1, 1> { public: - explicit LBranch(LOperand* value) { + explicit LBranch(LOperand* value, LOperand* temp) { inputs_[0] = value; + temps_[0] = temp; } DECLARE_CONCRETE_INSTRUCTION(Branch, "branch")