[interpreter] Remove redundant code in ForInPrepare

src/interpreter/interpreter.cc

 // Copyright 2015 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/interpreter/interpreter.h"
 
 #include <fstream>
 #include <memory>
 
 #include "src/ast/prettyprinter.h"
@@ skipping 1892 matching lines @@
                                           Node* cache_type, Node* cache_array,
                                           Node* cache_length,
                                           InterpreterAssembler* assembler) {
   __ StoreRegister(cache_type, output_register);
   output_register = __ NextRegister(output_register);
   __ StoreRegister(cache_array, output_register);
   output_register = __ NextRegister(output_register);
   __ StoreRegister(cache_length, output_register);
 }
 
 // ForInPrepare <cache_info_triple>

[rmcilroy, 2016/08/04 15:41:45]

I know you didn't change this, but could you update this comment to add
<reciever> operand and mention that it needs to be a JSReceiver object.

[klaasb, 2016/08/05 16:08:34]

Oops, I actually did that change, just in an earlier CL. Done.

 //
 // Returns state for for..in loop execution based on the object in the
 // accumulator. The result is output in registers |cache_info_triple| to
 // |cache_info_triple + 2|, with the registers holding cache_type, cache_array,
 // and cache_length respectively.
 void Interpreter::DoForInPrepare(InterpreterAssembler* assembler) {
   Node* object_reg = __ BytecodeOperandReg(0);
-  Node* object = __ LoadRegister(object_reg);
+  Node* receiver = __ LoadRegister(object_reg);
   Node* context = __ GetContext();
   Node* const zero_smi = __ SmiConstant(Smi::FromInt(0));
 

[rmcilroy, 2016/08/04 15:41:45]

Could you add some debug code (with "if (FLAG_debug_code) { ... }") which checks
that the code is a JS_RECEIVER and Aborts with the Abort(...) instruction with a
appropriate bailout (added one if needed).

[klaasb, 2016/08/05 16:08:34]

Done.

-  Label test_if_null(assembler), test_if_undefined(assembler),
-      nothing_to_iterate(assembler, Label::kDeferred),
-      convert_to_receiver(assembler, Label::kDeferred),
-      already_receiver(assembler), check_enum_cache(assembler);
+  Label nothing_to_iterate(assembler, Label::kDeferred),
+      use_enum_cache(assembler), use_runtime(assembler, Label::kDeferred);
 
-  Variable receiver(assembler, MachineRepresentation::kTagged);
-
-  // Test if object is already a receiver, no conversion necessary if so.
-  Node* instance_type = __ LoadInstanceType(object);
-  Node* first_receiver_type = __ Int32Constant(FIRST_JS_RECEIVER_TYPE);
-  __ BranchIfInt32GreaterThanOrEqual(instance_type, first_receiver_type,
-                                     &already_receiver, &test_if_null);
-
-  __ Bind(&test_if_null);
-  {
-    __ BranchIfWordEqual(object, assembler->NullConstant(), &nothing_to_iterate,
-                         &test_if_undefined);
-  }
-
-  __ Bind(&test_if_undefined);
-  {
-    __ BranchIfWordEqual(object, assembler->UndefinedConstant(),
-                         &nothing_to_iterate, &convert_to_receiver);

[rmcilroy, 2016/08/04 14:06:35]

I think we still need the null and undefined checks don't we? Or are they dealt
with by the CheckEnumCache? Either way, could you check the behaviour when null
/ undefined is past to this and make sure it still does the right thing.

[klaasb, 2016/08/04 15:20:54]

Null/undefined are also handled in bytecode already, so here we just expect a
non-null/undefined, converted receiver. CheckEnumCache would fail for
null/undefined as far as I can see.
There are test cases for null/undefined in the cctest
test-interpreter/InterpreterForIn that still hold.

-  }
-
-  __ Bind(&convert_to_receiver);
-  {
-    Callable callable = CodeFactory::ToObject(assembler->isolate());
-    Node* target = __ HeapConstant(callable.code());
-    Node* result = __ CallStub(callable.descriptor(), target, context, object);
-    receiver.Bind(result);
-    __ Goto(&check_enum_cache);
-  }
-
-  __ Bind(&already_receiver);
-  {
-    receiver.Bind(object);
-    __ Goto(&check_enum_cache);
-  }
-
-  Label use_enum_cache(assembler), use_runtime(assembler, Label::kDeferred);
-  __ Bind(&check_enum_cache);
-  { __ CheckEnumCache(receiver.value(), &use_enum_cache, &use_runtime); }
+  __ CheckEnumCache(receiver, &use_enum_cache, &use_runtime);
 
   __ Bind(&use_enum_cache);
   {
     // The enum cache is valid.  Load the map of the object being
     // iterated over and use the cache for the iteration.
-    Node* cache_type = __ LoadMap(receiver.value());
+    Node* cache_type = __ LoadMap(receiver);
     Node* cache_length = __ EnumLength(cache_type);
     __ GotoIf(assembler->WordEqual(cache_length, zero_smi),
               &nothing_to_iterate);
     Node* descriptors = __ LoadMapDescriptors(cache_type);
     Node* cache_offset =
         __ LoadObjectField(descriptors, DescriptorArray::kEnumCacheOffset);
     Node* cache_array = __ LoadObjectField(
         cache_offset, DescriptorArray::kEnumCacheBridgeCacheOffset);
     Node* output_register = __ BytecodeOperandReg(1);
     BuildForInPrepareResult(output_register, cache_type, cache_array,
                             cache_length, assembler);
     __ Dispatch();
   }
 
   __ Bind(&use_runtime);
   {
     Node* result_triple =
-        __ CallRuntime(Runtime::kForInPrepare, context, object);
+        __ CallRuntime(Runtime::kForInPrepare, context, receiver);
     Node* cache_type = __ Projection(0, result_triple);
     Node* cache_array = __ Projection(1, result_triple);
     Node* cache_length = __ Projection(2, result_triple);
     Node* output_register = __ BytecodeOperandReg(1);
     BuildForInPrepareResult(output_register, cache_type, cache_array,
                             cache_length, assembler);
     __ Dispatch();
   }
 
   __ Bind(&nothing_to_iterate);
@@ skipping 178 matching lines @@
   __ StoreObjectField(generator, JSGeneratorObject::kContinuationOffset,
       __ SmiTag(new_state));
   __ SetAccumulator(old_state);
 
   __ Dispatch();
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8