Inline array loads in loops directly in the code instead of always...

src/codegen-ia32.cc

 // Copyright 2006-2008 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 3760 matching lines @@
     SmiComparison(cc, right->AsLiteral()->handle(), strict);
     return;
   }
 
   Load(left);
   Load(right);
   Comparison(cc, strict);
 }
 
 
+class DeferredReferenceGetKeyedValue: public DeferredCode {
+ public:
+  DeferredReferenceGetKeyedValue(CodeGenerator* generator, bool is_global)
+      : DeferredCode(generator), is_global_(is_global) {
+    set_comment("[ DeferredReferenceGetKeyedValue");
+  }
+
+  virtual void Generate() {
+    Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));
+    // Calculate the delta from the IC call instruction to the map
+    // check cmp instruction in the inlined version.  This delta is
+    // stored in a test(eax, delta) instruction after the call so that
+    // we can find it in the IC initialization code and patch the cmp
+    // instruction.  This means that we cannot allow test instructions
+    // after calls to KeyedLoadIC stubs in other places.
+    int delta_to_patch_site = __ SizeOfCodeGeneratedSince(patch_site());
+    if (is_global_) {
+      __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
+    } else {
+      __ call(ic, RelocInfo::CODE_TARGET);
+    }
+    __ test(eax, Immediate(-delta_to_patch_site));
+    __ IncrementCounter(&Counters::keyed_load_inline_miss, 1);
+  }
+
+  Label* patch_site() { return &patch_site_; }
+
+ private:
+  Label patch_site_;
+  bool is_global_;
+};
+
+
+
 #undef __
 #define __ masm->
 
 Handle<String> Reference::GetName() {
   ASSERT(type_ == NAMED);
   Property* property = expression_->AsProperty();
   if (property == NULL) {
     // Global variable reference treated as a named property reference.
     VariableProxy* proxy = expression_->AsVariableProxy();
     ASSERT(proxy->AsVariable() != NULL);
@@ skipping 15 matching lines @@
   switch (type_) {
     case SLOT: {
       Comment cmnt(masm, "[ Load from Slot");
       Slot* slot = expression_->AsVariableProxy()->AsVariable()->slot();
       ASSERT(slot != NULL);
       cgen_->LoadFromSlot(slot, typeof_state);
       break;
     }
 
     case NAMED: {
-      // TODO(1241834): Make sure that this it is safe to ignore the
-      // distinction between expressions in a typeof and not in a typeof. If
-      // there is a chance that reference errors can be thrown below, we
-      // must distinguish between the two kinds of loads (typeof expression
-      // loads must not throw a reference error).
+      // TODO(1241834): Make sure that it is safe to ignore the
+      // distinction between expressions in a typeof and not in a
+      // typeof. If there is a chance that reference errors can be
+      // 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");
       Handle<String> name(GetName());
+      Variable* var = expression_->AsVariableProxy()->AsVariable();
       Handle<Code> ic(Builtins::builtin(Builtins::LoadIC_Initialize));
       // Setup the name register.
       __ mov(ecx, name);
-
-      Variable* var = expression_->AsVariableProxy()->AsVariable();
       if (var != NULL) {
         ASSERT(var->is_global());
         __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
       } else {
         __ call(ic, RelocInfo::CODE_TARGET);
       }
-      frame->Push(eax);  // IC call leaves result in eax, push it out
+      // Push the result.
+      frame->Push(eax);
       break;
     }
 
     case KEYED: {
-      // TODO(1241834): Make sure that this it is safe to ignore the
-      // distinction between expressions in a typeof and not in a typeof.
-      Comment cmnt(masm, "[ Load from keyed Property");
-      Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));
-
+      // TODO(1241834): Make sure that it is safe to ignore the
+      // distinction between expressions in a typeof and not in a
+      // typeof.
       Variable* var = expression_->AsVariableProxy()->AsVariable();
-      if (var != NULL) {
-        ASSERT(var->is_global());
-        __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
+      // Inline array load code if inside of a loop.  We do not know
+      // the receiver map yet, so we initially generate the code with
+      // a check against an invalid map.  In the inline cache code, we
+      // patch the map check if appropriate.
+      if (cgen_->loop_nesting() > 0) {
+        Comment cmnt(masm, "[ Inlined array index load");
+        DeferredReferenceGetKeyedValue* deferred =
+            new DeferredReferenceGetKeyedValue(cgen_, var != NULL);
+        // Load receiver and check that it is not a smi (only needed
+        // if not contextual) and that it has the expected map.

[iposva, 2008/12/22 18:59:16]

I think it would improve the readability if was clear that if var is not NULL,
that it is the global context. In other areas in this code we
ASSERT(var->is_gobal()) if var is not NULL.

+        __ mov(edx, Operand(esp, kPointerSize));
+        if (var == NULL) {
+          __ test(edx, Immediate(kSmiTagMask));
+          __ j(zero, deferred->enter(), not_taken);
+        }
+        // Initially, use an invalid map. The map is patched in the IC
+        // initialization code.
+        __ bind(deferred->patch_site());
+        __ cmp(FieldOperand(edx, HeapObject::kMapOffset),
+               Immediate(Factory::null_value()));
+        __ j(not_equal, deferred->enter(), not_taken);
+        // Load key and check that it is a smi.
+        __ mov(eax, Operand(esp, 0));
+        __ test(eax, Immediate(kSmiTagMask));
+        __ j(not_zero, deferred->enter(), not_taken);
+        // Shift to get actual index value.
+        __ sar(eax, kSmiTagSize);

[Kasper Lund, 2008/12/22 09:52:13]

Maybe we should consider keeping the FixedArray length as a smi instead of the
untagging in the fast case?

[Mads Ager (chromium), 2008/12/22 12:59:11]

That would be an interesting experiment.  I'll try that in another changelist.

+        // Get the elements array from the receiver and check that it
+        // is not a dictionary.
+        __ mov(edx, FieldOperand(edx, JSObject::kElementsOffset));
+        __ cmp(FieldOperand(edx, HeapObject::kMapOffset),
+               Immediate(Factory::hash_table_map()));
+        __ j(equal, deferred->enter(), not_taken);
+        // Check that key is within bounds.
+        __ cmp(eax, FieldOperand(edx, Array::kLengthOffset));
+        __ j(above_equal, deferred->enter(), not_taken);
+        // Load and check that the result is not the hole.
+        __ mov(eax,
+               Operand(edx, eax, times_4, Array::kHeaderSize - kHeapObjectTag));
+        __ cmp(Operand(eax), Immediate(Factory::the_hole_value()));
+        __ j(equal, deferred->enter(), not_taken);
+        __ IncrementCounter(&Counters::keyed_load_inline, 1);
+        __ bind(deferred->exit());
       } else {
-        __ call(ic, RelocInfo::CODE_TARGET);
+        Comment cmnt(masm, "[ Load from keyed Property");
+        Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));
+        if (var != NULL) {
+          ASSERT(var->is_global());
+          __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
+        } else {
+          __ call(ic, RelocInfo::CODE_TARGET);
+        }
+        // Make sure that we do not have a test instruction after the
+        // call.  A test instruction after the call is used to
+        // indicate that we have generated an inline version of the
+        // keyed load.
+        __ nop();

[Kasper Lund, 2008/12/22 09:52:13]

I guess here the nop is really needed because the following Push may be
peep-holed away. Maybe you should add that to the comment, because if you just
look at the code it seems like a non-test instruction will always follow the
call.

[Mads Ager (chromium), 2008/12/22 12:59:11]

Yes, exactly.  I'll update the comment to make it clear that the push is not
necessarily emitted.

[iposva, 2008/12/22 18:59:16]

Alternatively you could bind a label after the push if you really need the push
to be emitted. That way the the push cannot be combined with a potential pop.

       }
-      frame->Push(eax);  // IC call leaves result in eax, push it out
+      // Push the result.
+      frame->Push(eax);
       break;
     }
 
     default:
       UNREACHABLE();
   }
 }
 
 
 void Reference::SetValue(InitState init_state) {
@@ skipping 1326 matching lines @@
 
   // Slow-case: Go through the JavaScript implementation.
   __ bind(&slow);
   __ InvokeBuiltin(Builtins::INSTANCE_OF, JUMP_FUNCTION);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal