Optimize instanceof further...

src/ia32/code-stubs-ia32.cc

Index: src/ia32/code-stubs-ia32.cc
===================================================================
--- src/ia32/code-stubs-ia32.cc	(revision 6109)
+++ src/ia32/code-stubs-ia32.cc	(working copy)
@@ -4973,6 +4973,18 @@
 }
 
 
+// Generate stub code for instanceof.
+// This code can patch an call site inlined cache of the instance of check,

[Mads Ager (chromium), 2011/01/04 17:25:58]

an call site -> a call site

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+// which looks like this.
+//   cmp    edi, <the hole, patched to a map>
+//   jne    <some label>
+//   mov    eax, <the hole, patched to either true or false>
+// If that patching is requested the stack will have the delte from the return
+// address to the cmp instruction just just below the return address.

[Mads Ager (chromium), 2011/01/04 17:25:58]

just just -> just

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+//   esp[0] : return address
+//   esp[4] : delta from return address to cmp instruction

[Mads Ager (chromium), 2011/01/04 17:25:58]

You should add to this comment that call site patching only happens when args
are in registers. Begin the generation function with:

ASSERT(HasArgsInRegisters() || !HasCallSiteInlineCheck())

?

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+// This also means that for the call site patching the args have to be in
+// registers and not on the stack.
 void InstanceofStub::Generate(MacroAssembler* masm) {
   // Fixed register usage throughout the stub.
   Register object = eax;  // Object (lhs).
@@ -4981,9 +4993,14 @@
   Register prototype = edi;  // Prototype of the function.
   Register scratch = ecx;
 
+  ExternalReference roots_address = ExternalReference::roots_address();
+
+  ASSERT_EQ(object.code(), InstanceofStub::left().code());
+  ASSERT_EQ(function.code(), InstanceofStub::right().code());
+
   // Get the object and function - they are always both needed.
   Label slow, not_js_object;
-  if (!args_in_registers()) {
+  if (!HasArgsInRegisters()) {
     __ mov(object, Operand(esp, 2 * kPointerSize));
     __ mov(function, Operand(esp, 1 * kPointerSize));
   }
@@ -4993,22 +5010,26 @@
   __ j(zero, &not_js_object, not_taken);
   __ IsObjectJSObjectType(object, map, scratch, &not_js_object);
 
-  // Look up the function and the map in the instanceof cache.
-  NearLabel miss;
-  ExternalReference roots_address = ExternalReference::roots_address();
-  __ mov(scratch, Immediate(Heap::kInstanceofCacheFunctionRootIndex));
-  __ cmp(function,
-         Operand::StaticArray(scratch, times_pointer_size, roots_address));
-  __ j(not_equal, &miss);
-  __ mov(scratch, Immediate(Heap::kInstanceofCacheMapRootIndex));
-  __ cmp(map, Operand::StaticArray(scratch, times_pointer_size, roots_address));
-  __ j(not_equal, &miss);
-  __ mov(scratch, Immediate(Heap::kInstanceofCacheAnswerRootIndex));
-  __ mov(eax, Operand::StaticArray(scratch, times_pointer_size, roots_address));
-  __ IncrementCounter(&Counters::instance_of_cache, 1);
-  __ ret((args_in_registers() ? 0 : 2) * kPointerSize);
+  // If there is a call site cache don't look in the global cache, but do the
+  // real lookup and update the call site cache.
+  if (!HasCallSiteInlineCheck()) {
+    // Look up the function and the map in the instanceof cache.
+    NearLabel miss;
+    __ mov(scratch, Immediate(Heap::kInstanceofCacheFunctionRootIndex));
+    __ cmp(function,
+           Operand::StaticArray(scratch, times_pointer_size, roots_address));
+    __ j(not_equal, &miss);
+    __ mov(scratch, Immediate(Heap::kInstanceofCacheMapRootIndex));
+    __ cmp(map, Operand::StaticArray(
+        scratch, times_pointer_size, roots_address));
+    __ j(not_equal, &miss);
+    __ mov(scratch, Immediate(Heap::kInstanceofCacheAnswerRootIndex));
+    __ mov(eax, Operand::StaticArray(
+        scratch, times_pointer_size, roots_address));
+    __ ret((HasArgsInRegisters() ? 0 : 2) * kPointerSize);
+    __ bind(&miss);
+  }
 
-  __ bind(&miss);
   // Get the prototype of the function.
   __ TryGetFunctionPrototype(function, prototype, scratch, &slow);
 
@@ -5017,13 +5038,24 @@
   __ j(zero, &slow, not_taken);
   __ IsObjectJSObjectType(prototype, scratch, scratch, &slow);
 
-  // Update the golbal instanceof cache with the current map and function. The
+  // Update the global instanceof cache with the current map and function. The

[Mads Ager (chromium), 2011/01/04 17:25:58]

global -> global or inlined?

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

   // cached answer will be set when it is known.
+  if (!HasCallSiteInlineCheck()) {
   __ mov(scratch, Immediate(Heap::kInstanceofCacheMapRootIndex));
   __ mov(Operand::StaticArray(scratch, times_pointer_size, roots_address), map);
   __ mov(scratch, Immediate(Heap::kInstanceofCacheFunctionRootIndex));
   __ mov(Operand::StaticArray(scratch, times_pointer_size, roots_address),
          function);
+  } else {
+    // The constatns in for the code patching are based on no push instructions

[Mads Ager (chromium), 2011/01/04 17:25:58]

constatns in for -> constants for

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+    // at the call site.
+    ASSERT(HasArgsInRegisters());
+    // Get return address and delta to inlined map check.
+    __ mov(scratch, Operand(esp, 0 * kPointerSize));
+    __ sub(scratch, Operand(esp, 1 * kPointerSize));
+    __ add(Operand(scratch), Immediate(2));

[Mads Ager (chromium), 2011/01/04 17:25:58]

Named constant? Also, can't this be part of the operand in the move below?

Can you generate code when running with --debug-code that checks that you are
patching a cmp instruction here? Similarly for the moves that you patch below?

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+    __ mov(Operand(scratch, 0), map);
+  }
 
   // Loop through the prototype chain of the object looking for the function
   // prototype.
@@ -5039,18 +5071,42 @@
   __ jmp(&loop);
 
   __ bind(&is_instance);
-  __ IncrementCounter(&Counters::instance_of_stub_true, 1);
-  __ Set(eax, Immediate(0));
-  __ mov(scratch, Immediate(Heap::kInstanceofCacheAnswerRootIndex));
-  __ mov(Operand::StaticArray(scratch, times_pointer_size, roots_address), eax);
-  __ ret((args_in_registers() ? 0 : 2) * kPointerSize);
+  if (!HasCallSiteInlineCheck()) {
+    __ Set(eax, Immediate(0));
+    __ mov(scratch, Immediate(Heap::kInstanceofCacheAnswerRootIndex));
+    __ mov(Operand::StaticArray(scratch,
+                                times_pointer_size, roots_address), eax);
+  } else {
+    // Get return address and delte to inlined map check.
+    __ mov(eax, Factory::true_value());
+    __ mov(scratch, Operand(esp, 0 * kPointerSize));
+    __ sub(scratch, Operand(esp, 1 * kPointerSize));
+    __ add(Operand(scratch), Immediate(13));

[Mads Ager (chromium), 2011/01/04 17:25:58]

Named constant? Combine with move?

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+    __ mov(Operand(scratch, 0), eax);
+    if (!ReturnTrueFalseObject()) {
+      __ Set(eax, Immediate(0));
+    }
+  }
+  __ ret((HasArgsInRegisters() ? 0 : 2) * kPointerSize);
 
   __ bind(&is_not_instance);
-  __ IncrementCounter(&Counters::instance_of_stub_false, 1);
-  __ Set(eax, Immediate(Smi::FromInt(1)));
-  __ mov(scratch, Immediate(Heap::kInstanceofCacheAnswerRootIndex));
-  __ mov(Operand::StaticArray(scratch, times_pointer_size, roots_address), eax);
-  __ ret((args_in_registers() ? 0 : 2) * kPointerSize);
+  if (!HasCallSiteInlineCheck()) {
+    __ Set(eax, Immediate(Smi::FromInt(1)));
+    __ mov(scratch, Immediate(Heap::kInstanceofCacheAnswerRootIndex));
+    __ mov(Operand::StaticArray(
+        scratch, times_pointer_size, roots_address), eax);
+  } else {
+    // Get return address and delte to inlined map check.
+    __ mov(eax, Factory::false_value());
+    __ mov(scratch, Operand(esp, 0 * kPointerSize));
+    __ sub(scratch, Operand(esp, 1 * kPointerSize));
+    __ add(Operand(scratch), Immediate(13));

[Mads Ager (chromium), 2011/01/04 17:25:58]

Same here.

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+    __ mov(Operand(scratch, 0), eax);
+    if (!ReturnTrueFalseObject()) {
+      __ Set(eax, Immediate(Smi::FromInt(1)));
+    }
+  }
+  __ ret((HasArgsInRegisters() ? 0 : 2) * kPointerSize);
 
   Label object_not_null, object_not_null_or_smi;
   __ bind(&not_js_object);
@@ -5064,39 +5120,70 @@
   // Null is not instance of anything.
   __ cmp(object, Factory::null_value());
   __ j(not_equal, &object_not_null);
-  __ IncrementCounter(&Counters::instance_of_stub_false_null, 1);
   __ Set(eax, Immediate(Smi::FromInt(1)));
-  __ ret((args_in_registers() ? 0 : 2) * kPointerSize);
+  __ ret((HasArgsInRegisters() ? 0 : 2) * kPointerSize);
 
   __ bind(&object_not_null);
   // Smi values is not instance of anything.
   __ test(object, Immediate(kSmiTagMask));
   __ j(not_zero, &object_not_null_or_smi, not_taken);
   __ Set(eax, Immediate(Smi::FromInt(1)));
-  __ ret((args_in_registers() ? 0 : 2) * kPointerSize);
+  __ ret((HasArgsInRegisters() ? 0 : 2) * kPointerSize);
 
   __ bind(&object_not_null_or_smi);
   // String values is not instance of anything.
   Condition is_string = masm->IsObjectStringType(object, scratch, scratch);
   __ j(NegateCondition(is_string), &slow);
-  __ IncrementCounter(&Counters::instance_of_stub_false_string, 1);
   __ Set(eax, Immediate(Smi::FromInt(1)));
-  __ ret((args_in_registers() ? 0 : 2) * kPointerSize);
+  __ ret((HasArgsInRegisters() ? 0 : 2) * kPointerSize);
 
   // Slow-case: Go through the JavaScript implementation.
   __ bind(&slow);
-  if (args_in_registers()) {
+  if (HasArgsInRegisters()) {
     // Push arguments below return address.
     __ pop(scratch);
     __ push(object);
     __ push(function);
     __ push(scratch);
   }
-  __ IncrementCounter(&Counters::instance_of_slow, 1);
   __ InvokeBuiltin(Builtins::INSTANCE_OF, JUMP_FUNCTION);
 }
 
 
+Register InstanceofStub::left() { return eax; }
+

[Mads Ager (chromium), 2011/01/04 17:25:58]

two blank lines between functions

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+Register InstanceofStub::right() { return edx; }
+

[Mads Ager (chromium), 2011/01/04 17:25:58]

Two blanks.

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+const char* InstanceofStub::GetName() {
+  if (name_ != NULL) return name_;
+  const int kMaxNameLength = 100;
+  name_ = Bootstrapper::AllocateAutoDeletedArray(kMaxNameLength);
+  if (name_ == NULL) return "OOM";
+
+  const char* args = "";
+  if (HasArgsInRegisters()) {
+    args = "_REGS";
+  }
+
+  const char* inline_check = "";
+  if (HasCallSiteInlineCheck()) {
+    inline_check = "_INLINE";
+  }
+
+  const char* return_true_false_object = "";
+  if (ReturnTrueFalseObject()) {
+    return_true_false_object = "_TRUEFALSE";
+  }
+
+  OS::SNPrintF(Vector<char>(name_, kMaxNameLength),
+               "InstanceofStub%s%s",

[Mads Ager (chromium), 2011/01/04 17:25:58]

Isn't there a %s missing here?

[Søren Thygesen Gjesse, 2011/01/05 09:28:00]

Done.

+               args,
+               inline_check,
+               return_true_false_object);
+  return name_;
+}
+
+
 int CompareStub::MinorKey() {
   // Encode the three parameters in a unique 16 bit value. To avoid duplicate
   // stubs the never NaN NaN condition is only taken into account if the