From d30bc90b9cf82c477c8eb03ca5a987ed6e229722 Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Tue, 28 Apr 2009 10:40:36 +0000 Subject: [PATCH] Inline the inobject property case for named property loads. Review URL: http://codereview.chromium.org/99120 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1806 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/codegen.h | 3 ++ src/debug.cc | 4 +- src/ia32/codegen-ia32.cc | 112 ++++++++++++++++++++++++++++++++++++++++++----- src/ia32/ic-ia32.cc | 66 ++++++++++++++++++++-------- src/ic.cc | 30 ++++++++++++- src/ic.h | 16 +++++-- src/v8-counters.h | 2 + 7 files changed, 196 insertions(+), 37 deletions(-) diff --git a/src/codegen.h b/src/codegen.h index e9a5edd..0f60463 100644 --- a/src/codegen.h +++ b/src/codegen.h @@ -116,6 +116,9 @@ class DeferredCode: public ZoneObject { JumpTarget* enter() { return &enter_; } void BindExit() { exit_.Bind(0); } void BindExit(Result* result) { exit_.Bind(result, 1); } + void BindExit(Result* result0, Result* result1) { + exit_.Bind(result0, result1, 2); + } void BindExit(Result* result0, Result* result1, Result* result2) { exit_.Bind(result0, result1, result2, 3); } diff --git a/src/debug.cc b/src/debug.cc index 441a38e..50d8c80 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -377,9 +377,7 @@ void BreakLocationIterator::SetDebugBreakAtIC() { // is set the patching performed by the runtime system will take place in // the code copy and will therefore have no effect on the running code // keeping it from using the inlined code. - if (code->is_keyed_load_stub() && KeyedLoadIC::HasInlinedVersion(pc())) { - KeyedLoadIC::ClearInlinedVersion(pc()); - } + if (code->is_keyed_load_stub()) KeyedLoadIC::ClearInlinedVersion(pc()); } } diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc index 2d6fe83..69bd06c 100644 --- a/src/ia32/codegen-ia32.cc +++ b/src/ia32/codegen-ia32.cc @@ -30,6 +30,7 @@ #include "bootstrapper.h" #include "codegen-inl.h" #include "debug.h" +#include "ic-inl.h" #include "parser.h" #include "register-allocator-inl.h" #include "runtime.h" @@ -3410,7 +3411,10 @@ Result CodeGenerator::LoadFromGlobalSlotCheckExtensions( ? RelocInfo::CODE_TARGET : RelocInfo::CODE_TARGET_CONTEXT; Result answer = frame_->CallLoadIC(mode); - + // A test eax instruction following the call signals that the inobject + // property case was inlined. Ensure that there is not a test eax + // instruction here. + __ nop(); // Discard the global object. The result is in answer. frame_->Drop(); return answer; @@ -5233,6 +5237,47 @@ bool CodeGenerator::HasValidEntryRegisters() { #endif +class DeferredReferenceGetNamedValue: public DeferredCode { + public: + DeferredReferenceGetNamedValue(CodeGenerator* cgen, Handle name) + : DeferredCode(cgen), name_(name) { + set_comment("[ DeferredReferenceGetNamedValue"); + } + + virtual void Generate(); + + Label* patch_site() { return &patch_site_; } + + private: + Label patch_site_; + Handle name_; +}; + + +void DeferredReferenceGetNamedValue::Generate() { + CodeGenerator* cgen = generator(); + Result receiver(cgen); + enter()->Bind(&receiver); + + cgen->frame()->Push(&receiver); + cgen->frame()->Push(name_); + Result answer = cgen->frame()->CallLoadIC(RelocInfo::CODE_TARGET); + // The call must be followed by a test eax instruction to indicate + // that the inobject property case was inlined. + ASSERT(answer.is_register() && answer.reg().is(eax)); + // Store the delta to the map check instruction here in the test + // instruction. + int delta_to_patch_site = __ SizeOfCodeGeneratedSince(patch_site()); + // Here we use masm_-> instead of the double underscore macro because + // this is the instruction that gets patched and coverage code gets in + // the way. + masm_->test(answer.reg(), Immediate(-delta_to_patch_site)); + __ IncrementCounter(&Counters::named_load_inline_miss, 1); + receiver = cgen->frame()->Pop(); + exit_.Jump(&receiver, &answer); +} + + class DeferredReferenceGetKeyedValue: public DeferredCode { public: DeferredReferenceGetKeyedValue(CodeGenerator* generator, bool is_global) @@ -5334,16 +5379,63 @@ void Reference::GetValue(TypeofState typeof_state) { // thrown below, we must distinguish between the two kinds of // loads (typeof expression loads must not throw a reference // error). - Comment cmnt(masm, "[ Load from named Property"); - cgen_->frame()->Push(GetName()); - Variable* var = expression_->AsVariableProxy()->AsVariable(); - ASSERT(var == NULL || var->is_global()); - RelocInfo::Mode mode = (var == NULL) - ? RelocInfo::CODE_TARGET - : RelocInfo::CODE_TARGET_CONTEXT; - Result answer = cgen_->frame()->CallLoadIC(mode); - cgen_->frame()->Push(&answer); + bool is_global = var != NULL; + ASSERT(!is_global || var->is_global()); + + if (is_global || cgen_->scope()->is_global_scope()) { + // Do not inline the inobject property case for loads from the + // global object or loads in toplevel code. + Comment cmnt(masm, "[ Load from named Property"); + cgen_->frame()->Push(GetName()); + + RelocInfo::Mode mode = is_global + ? RelocInfo::CODE_TARGET_CONTEXT + : RelocInfo::CODE_TARGET; + Result answer = cgen_->frame()->CallLoadIC(mode); + // A test eax instruction following the call signals that the + // inobject property case was inlined. Ensure that there is not + // a test eax instruction here. + __ nop(); + cgen_->frame()->Push(&answer); + } else { + // Inline the inobject property case. + Comment cmnt(masm, "[ Inlined named property load"); + DeferredReferenceGetNamedValue* deferred = + new DeferredReferenceGetNamedValue(cgen_, GetName()); + Result receiver = cgen_->frame()->Pop(); + receiver.ToRegister(); + // Check that the receiver is a heap object. + __ test(receiver.reg(), Immediate(kSmiTagMask)); + deferred->enter()->Branch(zero, &receiver, not_taken); + + // Preallocate the value register to ensure that there is no + // spill emitted between the patch site label and the offset in + // the load instruction. + Result value = cgen_->allocator()->Allocate(); + ASSERT(value.is_valid()); + __ bind(deferred->patch_site()); + // This is the map check instruction that will be patched. + // Initially use an invalid map to force a failure. + __ cmp(FieldOperand(receiver.reg(), HeapObject::kMapOffset), + Immediate(Factory::null_value())); + deferred->enter()->Branch(not_equal, &receiver, not_taken); + + // The delta from the patch label to the load offset must be + // statically known. + ASSERT(masm->SizeOfCodeGeneratedSince(deferred->patch_site()) == + LoadIC::kOffsetToLoadInstruction); + // The initial (invalid) offset has to be large enough to force + // a 32-bit instruction encoding to allow patching with an + // arbitrary offset. Use kMaxInt (minus kHeapObjectTag). + int offset = kMaxInt; + __ mov(value.reg(), FieldOperand(receiver.reg(), offset)); + + __ IncrementCounter(&Counters::named_load_inline, 1); + deferred->BindExit(&receiver, &value); + cgen_->frame()->Push(&receiver); + cgen_->frame()->Push(&value); + } break; } diff --git a/src/ia32/ic-ia32.cc b/src/ia32/ic-ia32.cc index 559ac24..4231bfa 100644 --- a/src/ia32/ic-ia32.cc +++ b/src/ia32/ic-ia32.cc @@ -729,36 +729,66 @@ void LoadIC::Generate(MacroAssembler* masm, const ExternalReference& f) { static const byte kTestEaxByte = 0xA9; -bool KeyedLoadIC::HasInlinedVersion(Address address) { - Address test_instruction_address = address + 4; // 4 = stub address - return *test_instruction_address == kTestEaxByte; +void LoadIC::ClearInlinedVersion(Address address) { + // Reset the map check of the inlined inobject property load (if + // present) to guarantee failure by holding an invalid map (the null + // value). The offset can be patched to anything. + PatchInlinedLoad(address, Heap::null_value(), kMaxInt); } void KeyedLoadIC::ClearInlinedVersion(Address address) { // Insert null as the map to check for to make sure the map check fails // sending control flow to the IC instead of the inlined version. - PatchInlinedMapCheck(address, Heap::null_value()); + PatchInlinedLoad(address, Heap::null_value()); } -void KeyedLoadIC::PatchInlinedMapCheck(Address address, Object* value) { +bool LoadIC::PatchInlinedLoad(Address address, Object* map, int offset) { + // The address of the instruction following the call. + Address test_instruction_address = address + 4; + // If the instruction following the call is not a test eax, nothing + // was inlined. + if (*test_instruction_address != kTestEaxByte) return false; + + Address delta_address = test_instruction_address + 1; + // The delta to the start of the map check instruction. + int delta = *reinterpret_cast(delta_address); + + // The map address is the last 4 bytes of the 7-byte + // operand-immediate compare instruction, so we add 3 to get the + // offset to the last 4 bytes. + Address map_address = test_instruction_address + delta + 3; + *(reinterpret_cast(map_address)) = map; + + // The offset is in the last 4 bytes of a six byte + // memory-to-register move instruction, so we add 2 to get the + // offset to the last 4 bytes. + Address offset_address = + test_instruction_address + delta + kOffsetToLoadInstruction + 2; + *reinterpret_cast(offset_address) = offset - kHeapObjectTag; + return true; +} + + +bool KeyedLoadIC::PatchInlinedLoad(Address address, Object* map) { Address test_instruction_address = address + 4; // 4 = stub address // The keyed load has a fast inlined case if the IC call instruction // is immediately followed by a test instruction. - if (*test_instruction_address == kTestEaxByte) { - // Fetch the offset from the test instruction to the map cmp - // instruction. This offset is stored in the last 4 bytes of the - // 5 byte test instruction. - Address offset_address = test_instruction_address + 1; - int offset_value = *(reinterpret_cast(offset_address)); - // Compute the map address. The map address is in the last 4 - // bytes of the 7-byte operand-immediate compare instruction, so - // we add 3 to the offset to get the map address. - Address map_address = test_instruction_address + offset_value + 3; - // Patch the map check. - (*(reinterpret_cast(map_address))) = value; - } + if (*test_instruction_address != kTestEaxByte) return false; + + // Fetch the offset from the test instruction to the map cmp + // instruction. This offset is stored in the last 4 bytes of the 5 + // byte test instruction. + Address delta_address = test_instruction_address + 1; + int delta = *reinterpret_cast(delta_address); + // Compute the map address. The map address is in the last 4 bytes + // of the 7-byte operand-immediate compare instruction, so we add 3 + // to the offset to get the map address. + Address map_address = test_instruction_address + delta + 3; + // Patch the map check. + *(reinterpret_cast(map_address)) = map; + return true; } diff --git a/src/ic.cc b/src/ic.cc index 51768d7..ccdf3ca 100644 --- a/src/ic.cc +++ b/src/ic.cc @@ -42,7 +42,7 @@ static char TransitionMarkFromState(IC::State state) { switch (state) { case UNINITIALIZED: return '0'; case UNINITIALIZED_IN_LOOP: return 'L'; - case PREMONOMORPHIC: return '0'; + case PREMONOMORPHIC: return 'P'; case MONOMORPHIC: return '1'; case MONOMORPHIC_PROTOTYPE_FAILURE: return '^'; case MEGAMORPHIC: return 'N'; @@ -244,6 +244,7 @@ void KeyedLoadIC::Clear(Address address, Code* target) { void LoadIC::Clear(Address address, Code* target) { if (target->ic_state() == UNINITIALIZED) return; + ClearInlinedVersion(address); SetTargetAtAddress(address, initialize_stub()); } @@ -523,6 +524,31 @@ Object* LoadIC::Load(State state, Handle object, Handle name) { LOG(SuspectReadEvent(*name, *object)); } + bool can_be_inlined = + FLAG_use_ic && + state == PREMONOMORPHIC && + lookup.IsValid() && + lookup.IsLoaded() && + lookup.IsCacheable() && + lookup.holder() == *object && + lookup.type() == FIELD && + !object->IsAccessCheckNeeded(); + + if (can_be_inlined) { + Map* map = lookup.holder()->map(); + // Property's index in the properties array. If negative we have + // an inobject property. + int index = lookup.GetFieldIndex() - map->inobject_properties(); + if (index < 0) { + // Index is an offset from the end of the object. + int offset = map->instance_size() + (index * kPointerSize); + if (PatchInlinedLoad(address(), map, offset)) { + set_target(megamorphic_stub()); + return lookup.holder()->FastPropertyAt(lookup.GetFieldIndex()); + } + } + } + // Update inline cache and stub cache. if (FLAG_use_ic && lookup.IsLoaded()) { UpdateCaches(&lookup, state, object, name); @@ -734,7 +760,7 @@ Object* KeyedLoadIC::Load(State state, !object->IsJSValue() && !JSObject::cast(*object)->HasIndexedInterceptor()) { Map* map = JSObject::cast(*object)->map(); - PatchInlinedMapCheck(address(), map); + PatchInlinedLoad(address(), map); } } diff --git a/src/ic.h b/src/ic.h index 11d47ae..11fd604 100644 --- a/src/ic.h +++ b/src/ic.h @@ -216,6 +216,11 @@ class LoadIC: public IC { static void GenerateStringLength(MacroAssembler* masm); static void GenerateFunctionPrototype(MacroAssembler* masm); + // The offset from the inlined patch site to the start of the + // inlined load instruction. It is 7 bytes (test eax, imm) plus + // 6 bytes (jne slow_label). + static const int kOffsetToLoadInstruction = 13; + private: static void Generate(MacroAssembler* masm, const ExternalReference& f); @@ -238,6 +243,12 @@ class LoadIC: public IC { } static void Clear(Address address, Code* target); + + // Clear the use of the inlined version. + static void ClearInlinedVersion(Address address); + + static bool PatchInlinedLoad(Address address, Object* map, int index); + friend class IC; }; @@ -254,9 +265,6 @@ class KeyedLoadIC: public IC { static void GeneratePreMonomorphic(MacroAssembler* masm); static void GenerateGeneric(MacroAssembler* masm); - // Check if this IC corresponds to an inlined version. - static bool HasInlinedVersion(Address address); - // Clear the use of the inlined version. static void ClearInlinedVersion(Address address); @@ -287,7 +295,7 @@ class KeyedLoadIC: public IC { // Support for patching the map that is checked in an inlined // version of keyed load. - static void PatchInlinedMapCheck(Address address, Object* map); + static bool PatchInlinedLoad(Address address, Object* map); friend class IC; }; diff --git a/src/v8-counters.h b/src/v8-counters.h index 6596dbf..404b689 100644 --- a/src/v8-counters.h +++ b/src/v8-counters.h @@ -116,6 +116,8 @@ namespace v8 { namespace internal { SC(keyed_load_interceptor, V8.KeyedLoadInterceptor) \ SC(keyed_load_inline, V8.KeyedLoadInline) \ SC(keyed_load_inline_miss, V8.KeyedLoadInlineMiss) \ + SC(named_load_inline, V8.NamedLoadInline) \ + SC(named_load_inline_miss, V8.NamedLoadInlineMiss) \ SC(keyed_store_field, V8.KeyedStoreField) \ SC(for_in, V8.ForIn) \ SC(enum_cache_hits, V8.EnumCacheHits) \ -- 2.7.4