Hydrogen: fix keyed loads with string keys

src/ic/ic-compiler.cc

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/v8.h"
 
 #include "src/ic/handler-compiler.h"
 #include "src/ic/ic-inl.h"
 #include "src/ic/ic-compiler.h"
 
@@ skipping 45 matching lines @@
   }
 
   CacheHolderFlag flag;
   Handle<Map> stub_holder = IC::GetICCacheHolder(*type, isolate, &flag);
   if (kind == Code::KEYED_STORE_IC) {
     // Always set the "property" bit.
     extra_ic_state =
         KeyedStoreIC::IcCheckTypeField::update(extra_ic_state, PROPERTY);
     DCHECK(STANDARD_STORE ==
            KeyedStoreIC::GetKeyedAccessStoreMode(extra_ic_state));
+  } else if (kind == Code::KEYED_LOAD_IC) {
+    extra_ic_state = KeyedLoadIC::IcCheckTypeField::update(extra_ic_state,
+                                                           PROPERTY);
   }
 
   Handle<Code> ic;
   // There are multiple string maps that all use the same prototype. That
   // prototype cannot hold multiple handlers, one for each of the string maps,
   // for a single name. Hence, turn off caching of the IC.
   bool can_be_cached = !type->Is(HeapType::String());
   if (can_be_cached) {
     ic = Find(name, stub_holder, kind, extra_ic_state, flag);
     if (!ic.is_null()) return ic;
   }
 
   PropertyICCompiler ic_compiler(isolate, kind, extra_ic_state, flag);
   ic = ic_compiler.CompileMonomorphic(type, handler, name, PROPERTY);
 
   if (can_be_cached) Map::UpdateCodeCache(stub_holder, name, ic);
   return ic;
 }
 
 
 Handle<Code> PropertyICCompiler::ComputeKeyedLoadMonomorphic(
-    Handle<Map> receiver_map) {
+    Handle<Map> receiver_map,
+    ExtraICState extra_ic_state,
+    IcCheckType key_type) {
   Isolate* isolate = receiver_map->GetIsolate();
-  Code::Flags flags = Code::ComputeMonomorphicFlags(Code::KEYED_LOAD_IC);
+  extra_ic_state = KeyedLoadIC::IcCheckTypeField::update(extra_ic_state,
+                                                         key_type);
+  Code::Flags flags = Code::ComputeMonomorphicFlags(Code::KEYED_LOAD_IC,
+                                                    extra_ic_state);
   Handle<Name> name = isolate->factory()->KeyedLoadMonomorphic_string();
 
   Handle<Object> probe(receiver_map->FindInCodeCache(*name, flags), isolate);
   if (probe->IsCode()) return Handle<Code>::cast(probe);
 
   Handle<Code> stub = ComputeKeyedLoadMonomorphicHandler(receiver_map);
-  PropertyICCompiler compiler(isolate, Code::KEYED_LOAD_IC);
+  PropertyICCompiler compiler(isolate, Code::KEYED_LOAD_IC, extra_ic_state);
   Handle<Code> code =
       compiler.CompileMonomorphic(HeapType::Class(receiver_map, isolate), stub,
                                   isolate->factory()->empty_string(), ELEMENT);

[Jakob Kummerow, 2014/12/11 09:52:42]

See how it says ELEMENT here? This method is only used to compile ELEMENT access
ICs, and that's the only thing it *can* handle, so there's really no need to
pretend that it's more flexible than that. Let's not add unnecessary complexity.

 
   Map::UpdateCodeCache(receiver_map, name, code);
   return code;
 }
 
 
 Handle<Code> PropertyICCompiler::ComputeKeyedLoadMonomorphicHandler(
     Handle<Map> receiver_map) {
   Isolate* isolate = receiver_map->GetIsolate();
   ElementsKind elements_kind = receiver_map->elements_kind();
@@ skipping 136 matching lines @@
   if (!receiver_map->is_dictionary_map()) {
     Map::UpdateCodeCache(receiver_map, name, ic);
   }
 
   return ic;
 }
 
 
 // TODO(verwaest): Change this method so it takes in a TypeHandleList.
 Handle<Code> PropertyICCompiler::ComputeKeyedLoadPolymorphic(
-    MapHandleList* receiver_maps) {
+    MapHandleList* receiver_maps,
+    ExtraICState extra_ic_state,
+    IcCheckType key_type) {
   Isolate* isolate = receiver_maps->at(0)->GetIsolate();
-  Code::Flags flags = Code::ComputeFlags(Code::KEYED_LOAD_IC, POLYMORPHIC);
+  extra_ic_state = KeyedLoadIC::IcCheckTypeField::update(extra_ic_state,
+                                                         key_type);
+  Code::Flags flags = Code::ComputeFlags(Code::KEYED_LOAD_IC,
+                                         POLYMORPHIC,
+                                         extra_ic_state);
   Handle<PolymorphicCodeCache> cache =
       isolate->factory()->polymorphic_code_cache();
   Handle<Object> probe = cache->Lookup(receiver_maps, flags);
   if (probe->IsCode()) return Handle<Code>::cast(probe);
 
   TypeHandleList types(receiver_maps->length());
   for (int i = 0; i < receiver_maps->length(); i++) {
     types.Add(HeapType::Class(receiver_maps->at(i), isolate));
   }
   CodeHandleList handlers(receiver_maps->length());
   ElementHandlerCompiler compiler(isolate);
   compiler.CompileElementHandlers(receiver_maps, &handlers);
-  PropertyICCompiler ic_compiler(isolate, Code::KEYED_LOAD_IC);
+  PropertyICCompiler ic_compiler(isolate, Code::KEYED_LOAD_IC, extra_ic_state);
   Handle<Code> code = ic_compiler.CompilePolymorphic(
       &types, &handlers, isolate->factory()->empty_string(), Code::NORMAL,
-      ELEMENT);
+      key_type);

[Jakob Kummerow, 2014/12/11 09:52:41]

So here you're replacing the hard-coded ELEMENT with the supposedly variable
key_type, but at the only call site of this function, ELEMENT is always passed
in. Again, let's not add complexity that's not required.

 
   isolate->counters()->keyed_load_polymorphic_stubs()->Increment();
 
   PolymorphicCodeCache::Update(cache, receiver_maps, flags, code);
   return code;
 }
 
 
 Handle<Code> PropertyICCompiler::ComputePolymorphic(
     Code::Kind kind, TypeHandleList* types, CodeHandleList* handlers,
@@ skipping 164 matching lines @@
 
   TailCallBuiltin(masm(), Builtins::kKeyedStoreIC_Miss);
 
   return GetCode(kind(), Code::NORMAL, factory()->empty_string());
 }
 
 
 #undef __
 }
 }  // namespace v8::internal