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) {

[Jakob Kummerow, 2014/12/12 10:14:27]

If you insist on keeping this additional argument, please explain to me in which
cases it changes anything. I still don't think we need it, and I don't like
adding extra code and extra parameters that don't do anything.
(Same below in the polymorphic case.)

[indutny, 2014/12/12 13:05:37]

I'd either need to pass this, or StrictMode to be able to generate
extra_ic_state here. Which do you prefer?

[Jakob Kummerow, 2014/12/12 13:34:16]

Why do you need StrictMode? Loads don't depend on strict mode. We didn't need to
pass in anything before, and we don't want to change behavior for element loads
(or the way their stubs are compiled/cached/retrieved), and we also don't want
to toggle that bit arbitrarily: valid transitions are uninitialized -> ELEMENT
-> generic and uninitialized -> PROPERTY -> generic, but never ELEMENT ->
PROPERTY -> ELEMENT -> PROPERTY -> etc.

So why would we need to pass in anything now?

   Isolate* isolate = receiver_map->GetIsolate();
-  Code::Flags flags = Code::ComputeMonomorphicFlags(Code::KEYED_LOAD_IC);
+  extra_ic_state = KeyedLoadIC::IcCheckTypeField::update(extra_ic_state,
+                                                         ELEMENT);
+  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);
 
   Map::UpdateCodeCache(receiver_map, name, code);
   return code;
 }
 
 
 Handle<Code> PropertyICCompiler::ComputeKeyedLoadMonomorphicHandler(
@@ skipping 139 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) {
   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,
+                                                         ELEMENT);
+  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);
 
   isolate->counters()->keyed_load_polymorphic_stubs()->Increment();
 
   PolymorphicCodeCache::Update(cache, receiver_maps, flags, code);
   return code;
 }
 
@@ skipping 167 matching lines @@
 
   TailCallBuiltin(masm(), Builtins::kKeyedStoreIC_Miss);
 
   return GetCode(kind(), Code::NORMAL, factory()->empty_string());
 }
 
 
 #undef __
 }
 }  // namespace v8::internal