Slight cleanup/unification of UpdateCache code.

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index 8b3b4723fe215f6ad68566d6a7c3ed70067c3e14..8dbe9094abe130b63c00a8049058e3feb487dcbd 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -712,7 +712,6 @@ void CallICBase::UpdateCaches(LookupResult* lookup,
 
   // Compute the number of arguments.
   int argc = target()->arguments_count();
-  bool had_proto_failure = false;
   Handle<Code> code;
   if (state == UNINITIALIZED) {
     // This is the first time we execute this inline cache.
@@ -729,7 +728,7 @@ void CallICBase::UpdateCaches(LookupResult* lookup,
                TryRemoveInvalidPrototypeDependentStub(target(),
                                                       *object,
                                                       *name)) {
-      had_proto_failure = true;
+      state = MONOMORPHIC_PROTOTYPE_FAILURE;
       code = ComputeMonomorphicStub(lookup, state, extra_ic_state,
                                     object, name);
     } else {
@@ -745,22 +744,36 @@ void CallICBase::UpdateCaches(LookupResult* lookup,
   if (code.is_null()) return;
 
   // Patch the call site depending on the state of the cache.
-  if (state == UNINITIALIZED ||
-      state == PREMONOMORPHIC ||
-      state == MONOMORPHIC ||
-      state == MONOMORPHIC_PROTOTYPE_FAILURE) {
-    set_target(*code);
-  } else if (state == MEGAMORPHIC) {
-    // Cache code holding map should be consistent with
-    // GenerateMonomorphicCacheProbe. It is not the map which holds the stub.
-    Handle<JSObject> cache_object = object->IsJSObject()
-        ? Handle<JSObject>::cast(object)
-        : Handle<JSObject>(JSObject::cast(object->GetPrototype()));
-    // Update the stub cache.
-    isolate()->stub_cache()->Set(*name, cache_object->map(), *code);
-  }
-
-  if (had_proto_failure) state = MONOMORPHIC_PROTOTYPE_FAILURE;
+  switch (state) {
+    case UNINITIALIZED:
+    case MONOMORPHIC_PROTOTYPE_FAILURE:
+    case PREMONOMORPHIC:
+      set_target(*code);
+      break;
+    case MONOMORPHIC:
+      if (code->ic_state() != MONOMORPHIC) {
+        Map* map = target()->FindFirstMap();

[ulan, 2013/01/07 15:04:54]

Can we add a comment here like in LoadIC case?

Any reason why we use different checks here (code->ic_state() != MONOMORPHIC)
and in LoadIC (target() != *code) ?

[Toon Verwaest, 2013/01/07 15:28:45]

The code above calculates the target different from the other code. It may
update the monomorphic stub, thus staying monomorphic. It will already assign
code to be a megamorphic stub if it needs to become megamorphic in the code
above. Hence, we only update the cache if that code has decided to become
megamorphic.

On 2013/01/07 15:04:54, ulan wrote:

+        if (map != NULL) {
+          isolate()->stub_cache()->Set(*name, map, target());
+        }
+      }
+      set_target(*code);
+      break;
+    case MEGAMORPHIC: {
+      // Cache code holding map should be consistent with
+      // GenerateMonomorphicCacheProbe. It is not the map which holds the stub.
+      Handle<JSObject> cache_object = object->IsJSObject()
+          ? Handle<JSObject>::cast(object)
+          : Handle<JSObject>(JSObject::cast(object->GetPrototype()));
+      // Update the stub cache.
+      isolate()->stub_cache()->Set(*name, cache_object->map(), *code);
+      break;
+    }
+    case DEBUG_BREAK:
+    case DEBUG_PREPARE_STEP_IN:
+      break;
+  }
+
   TRACE_IC(kind_ == Code::CALL_IC ? "CallIC" : "KeyedCallIC",
            name, state, target());
 }
@@ -1024,25 +1037,33 @@ void LoadIC::UpdateCaches(LookupResult* lookup,
   }
 
   // Patch the call site depending on the state of the cache.
-  if (state == UNINITIALIZED ||
-      state == PREMONOMORPHIC ||
-      state == MONOMORPHIC_PROTOTYPE_FAILURE) {
-    set_target(*code);
-  } else if (state == MONOMORPHIC) {
-    // We are transitioning from monomorphic to megamorphic case.
-    // Place the current monomorphic stub and stub compiled for
-    // the receiver into stub cache.
-    Map* map = target()->FindFirstMap();
-    if (map != NULL) {
-      isolate()->stub_cache()->Set(*name, map, target());
-    }
-    isolate()->stub_cache()->Set(*name, receiver->map(), *code);
+  switch (state) {
+    case UNINITIALIZED:
+    case PREMONOMORPHIC:
+    case MONOMORPHIC_PROTOTYPE_FAILURE:
+      set_target(*code);
+      break;
+    case MONOMORPHIC:
+      if (target() != *code) {
+        // We are transitioning from monomorphic to megamorphic case.
+        // Place the current monomorphic stub and stub compiled for
+        // the receiver into stub cache.
+        Map* map = target()->FindFirstMap();
+        if (map != NULL) {
+          isolate()->stub_cache()->Set(*name, map, target());
+        }
+        isolate()->stub_cache()->Set(*name, receiver->map(), *code);
 
-    set_target(*megamorphic_stub());
-  } else if (state == MEGAMORPHIC) {
-    // Cache code holding map should be consistent with
-    // GenerateMonomorphicCacheProbe.
-    isolate()->stub_cache()->Set(*name, receiver->map(), *code);
+        set_target(*megamorphic_stub());
+      }
+      break;
+    case MEGAMORPHIC:
+      // Cache code holding map should be consistent with
+      // GenerateMonomorphicCacheProbe.
+      isolate()->stub_cache()->Set(*name, receiver->map(), *code);

[ulan, 2013/01/07 15:04:54]

missing break.

[Toon Verwaest, 2013/01/07 15:28:45]

Done.

+    case DEBUG_BREAK:
+    case DEBUG_PREPARE_STEP_IN:
+      break;
   }
 
   TRACE_IC("LoadIC", name, state, target());
@@ -1296,13 +1317,24 @@ void KeyedLoadIC::UpdateCaches(LookupResult* lookup,
     }
   }
 
-  // Patch the call site depending on the state of the cache.  Make
-  // sure to always rewrite from monomorphic to megamorphic.
-  ASSERT(state != MONOMORPHIC_PROTOTYPE_FAILURE);
-  if (state == UNINITIALIZED || state == PREMONOMORPHIC) {
-    set_target(*code);
-  } else if (state == MONOMORPHIC) {
-    set_target(*megamorphic_stub());
+  // Patch the call site depending on the state of the cache.
+  switch (state) {
+    case UNINITIALIZED:
+    case PREMONOMORPHIC:
+      set_target(*code);
+      break;
+    case MONOMORPHIC:
+      // Only move to megamorphic if the target changes.
+      if (target() != *code) {
+        set_target(*megamorphic_stub());
+      }

[ulan, 2013/01/07 15:04:54]

missing break.

[Toon Verwaest, 2013/01/07 15:28:45]

Done.

+    case MEGAMORPHIC:
+    case DEBUG_BREAK:
+    case DEBUG_PREPARE_STEP_IN:
+      break;
+    case MONOMORPHIC_PROTOTYPE_FAILURE:
+      UNREACHABLE();
+      break;
   }
 
   TRACE_IC("KeyedLoadIC", name, state, target());
@@ -1547,18 +1579,32 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
   }
 
   // Patch the call site depending on the state of the cache.
-  if (state == UNINITIALIZED || state == MONOMORPHIC_PROTOTYPE_FAILURE) {
-    set_target(*code);
-  } else if (state == MONOMORPHIC) {
-    // Only move to megamorphic if the target changes.
-    if (target() != *code) {
-      set_target((strict_mode == kStrictMode)
-                   ? megamorphic_stub_strict()
-                   : megamorphic_stub());
-    }
-  } else if (state == MEGAMORPHIC) {
-    // Update the stub cache.
-    isolate()->stub_cache()->Set(*name, receiver->map(), *code);
+  switch (state) {
+    case UNINITIALIZED:
+    case PREMONOMORPHIC:
+    case MONOMORPHIC_PROTOTYPE_FAILURE:
+      set_target(*code);
+      break;
+    case MONOMORPHIC:
+      // Only move to megamorphic if the target changes.
+      if (target() != *code) {
+        Map* map = target()->FindFirstMap();
+        if (map != NULL) {
+          isolate()->stub_cache()->Set(*name, map, target());

[ulan, 2013/01/07 15:04:54]

Can we add a comment explaining this stub cache updates like in LoadIC case?

[Toon Verwaest, 2013/01/07 15:28:45]

Done.

+        }
+        isolate()->stub_cache()->Set(*name, receiver->map(), *code);
+        set_target((strict_mode == kStrictMode)
+                     ? megamorphic_stub_strict()
+                     : megamorphic_stub());
+      }
+      break;
+    case MEGAMORPHIC:
+      // Update the stub cache.
+      isolate()->stub_cache()->Set(*name, receiver->map(), *code);
+      break;
+    case DEBUG_BREAK:
+    case DEBUG_PREPARE_STEP_IN:
+      break;
   }
 
   TRACE_IC("StoreIC", name, state, target());
@@ -1585,18 +1631,28 @@ void KeyedIC::GetReceiverMapsForStub(Handle<Code> stub,
   if (!string_stub().is_null() && stub.is_identical_to(string_stub())) {
     return result->Add(isolate()->factory()->string_map());
   } else if (stub->is_keyed_load_stub() || stub->is_keyed_store_stub()) {
-    if (stub->ic_state() == MONOMORPHIC) {
-      result->Add(Handle<Map>(stub->FindFirstMap()));
-    } else {
-      ASSERT(stub->ic_state() == MEGAMORPHIC);
-      AssertNoAllocation no_allocation;
-      int mask = RelocInfo::ModeMask(RelocInfo::EMBEDDED_OBJECT);
-      for (RelocIterator it(*stub, mask); !it.done(); it.next()) {
-        RelocInfo* info = it.rinfo();
-        Handle<Object> object(info->target_object());
-        ASSERT(object->IsMap());
-        AddOneReceiverMapIfMissing(result, Handle<Map>::cast(object));
+    switch (stub->ic_state()) {
+      case MONOMORPHIC:
+        result->Add(Handle<Map>(stub->FindFirstMap()));
+        break;
+      case MEGAMORPHIC: {
+        AssertNoAllocation no_allocation;
+        int mask = RelocInfo::ModeMask(RelocInfo::EMBEDDED_OBJECT);
+        for (RelocIterator it(*stub, mask); !it.done(); it.next()) {
+          RelocInfo* info = it.rinfo();
+          Handle<Object> object(info->target_object());
+          ASSERT(object->IsMap());
+          AddOneReceiverMapIfMissing(result, Handle<Map>::cast(object));
+        }
+        break;
       }
+      case UNINITIALIZED:
+      case PREMONOMORPHIC:
+      case MONOMORPHIC_PROTOTYPE_FAILURE:
+      case DEBUG_BREAK:
+      case DEBUG_PREPARE_STEP_IN:
+        UNREACHABLE();
+        break;
     }
   }
 }
@@ -2024,15 +2080,27 @@ void KeyedStoreIC::UpdateCaches(LookupResult* lookup,
 
   ASSERT(!code.is_null());
 
-  // Patch the call site depending on the state of the cache.  Make
-  // sure to always rewrite from monomorphic to megamorphic.
-  ASSERT(state != MONOMORPHIC_PROTOTYPE_FAILURE);
-  if (state == UNINITIALIZED || state == PREMONOMORPHIC) {
-    set_target(*code);
-  } else if (state == MONOMORPHIC) {
-    set_target((strict_mode == kStrictMode)
-                 ? *megamorphic_stub_strict()
-                 : *megamorphic_stub());
+  // Patch the call site depending on the state of the cache.
+  switch (state) {
+    case UNINITIALIZED:
+    case PREMONOMORPHIC:
+      set_target(*code);
+      break;
+    case MONOMORPHIC:
+      // Only move to megamorphic if the target changes.
+      if (target() != *code) {
+        set_target((strict_mode == kStrictMode)
+                     ? *megamorphic_stub_strict()
+                     : *megamorphic_stub());
+      }
+      break;
+    case MEGAMORPHIC:
+    case DEBUG_BREAK:
+    case DEBUG_PREPARE_STEP_IN:
+      break;
+    case MONOMORPHIC_PROTOTYPE_FAILURE:
+      UNREACHABLE();
+      break;
   }
 
   TRACE_IC("KeyedStoreIC", name, state, target());
@@ -2057,13 +2125,12 @@ RUNTIME_FUNCTION(MaybeObject*, CallIC_Miss) {
                                               extra_ic_state,
                                               args.at<Object>(0),
                                               args.at<String>(1));
-  // Result could be a function or a failure.
-  JSFunction* raw_function = NULL;

[ulan, 2013/01/07 15:04:54]

I am not sure, might be that we need it to avoid compiler warnings on some
platform.

[Toon Verwaest, 2013/01/07 15:28:45]

I doubt it. It would be the only maybe->To(&value) site to require this... There
are many different legacy versions of this idiom lying around the codebase; I'm
just cleaning them up as I see them ;-)

On 2013/01/07 15:04:54, ulan wrote:

+  JSFunction* raw_function;
   if (!maybe_result->To(&raw_function)) return maybe_result;
 
   // The first time the inline cache is updated may be the first time the
-  // function it references gets called.  If the function is lazily compiled
-  // then the first call will trigger a compilation.  We check for this case
+  // function it references gets called. If the function is lazily compiled
+  // then the first call will trigger a compilation. We check for this case
   // and we do the compilation immediately, instead of waiting for the stub
   // currently attached to the JSFunction object to trigger compilation.
   if (raw_function->is_compiled()) return raw_function;