Add Crankshaft support for smi-only elements

src/hydrogen.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 3319 matching lines @@
     // Load the elements array before the first store.
     if (elements == NULL)  {
       elements = new(zone()) HLoadElements(literal);
       AddInstruction(elements);
     }
 
     HValue* key = AddInstruction(
         new(zone()) HConstant(Handle<Object>(Smi::FromInt(i)),
                               Representation::Integer32()));
     if (FLAG_smi_only_arrays) {
+      HInstruction* elements_kind =
+          AddInstruction(new(zone()) HElementsKind(literal));
+      HBasicBlock* store_fast = graph()->CreateBasicBlock();

[danno, 2011/09/23 09:05:24]

Wow, cool, you went for bonus points here (I don't know if this is a perf
bottneck or not). It's probably a good idea to include a unit test that
explicitly triggers this logic, since it is now more than a one-liner.

[Jakob Kummerow, 2011/09/26 11:30:32]

Done.

+      // Two empty blocks to satisfy edge split form.
+      HBasicBlock* store_fast_edgesplit1 = graph()->CreateBasicBlock();
+      HBasicBlock* store_fast_edgesplit2 = graph()->CreateBasicBlock();
+      HBasicBlock* store_generic = graph()->CreateBasicBlock();
+      HBasicBlock* check_smi_only_elements = graph()->CreateBasicBlock();
+      HBasicBlock* join = graph()->CreateBasicBlock();
+
+      HIsSmiAndBranch* smicheck = new(zone()) HIsSmiAndBranch(value);
+      smicheck->SetSuccessorAt(0, store_fast_edgesplit1);
+      smicheck->SetSuccessorAt(1, check_smi_only_elements);
+      current_block()->Finish(smicheck);
+      store_fast_edgesplit1->Finish(new(zone()) HGoto(store_fast));
+
+      set_current_block(check_smi_only_elements);
+      HCompareConstantEqAndBranch* smi_elements_check =
+          new(zone()) HCompareConstantEqAndBranch(elements_kind,
+                                                  FAST_SMI_ONLY_ELEMENTS,
+                                                  Token::EQ_STRICT);
+      smi_elements_check->SetSuccessorAt(0, store_generic);
+      smi_elements_check->SetSuccessorAt(1, store_fast_edgesplit2);
+      current_block()->Finish(smi_elements_check);
+      store_fast_edgesplit2->Finish(new(zone()) HGoto(store_fast));
+
+      set_current_block(store_fast);
+      AddInstruction(new(zone()) HStoreKeyedFastElement(elements, key, value));
+      store_fast->Goto(join);
+
+      set_current_block(store_generic);
       AddInstruction(BuildStoreKeyedGeneric(literal, key, value));
+      store_generic->Goto(join);
+
+      join->SetJoinId(expr->id());
+      set_current_block(join);
     } else {
       AddInstruction(new(zone()) HStoreKeyedFastElement(elements, key, value));
     }
 
     AddSimulate(expr->GetIdForElement(i));
   }
   return ast_context()->ReturnValue(Pop());
 }
 
 
@@ skipping 611 matching lines @@
     }
     return new(zone()) HStoreKeyedSpecializedArrayElement(
         external_elements, checked_key, val, elements_kind);
   } else {
     return new(zone()) HLoadKeyedSpecializedArrayElement(
         external_elements, checked_key, elements_kind);
   }
 }
 
 
+HInstruction* HGraphBuilder::BuildFastElementAccess(HValue* elements,

[danno, 2011/09/23 09:05:24]

I find it a little asymmetric that BuildFirstElementAccess adds the SmiCheck,
but not the last one. Maybe instead it should always to the AddInstruction for
the result instruction, and return a HValue* (or NULL for stores)?

[danno, 2011/09/23 09:07:52]

"not last last one" should read "the returned instruction"
On 2011/09/23 09:05:24, danno wrote:

[Jakob Kummerow, 2011/09/26 11:30:32]

It's currently consistent with a bunch of other methods in hydrogen.cc. I can
refactor it everywhere, but would prefer to do that in a separate CL as it's
unrelated.

+                                                    HValue* checked_key,
+                                                    HValue* val,
+                                                    ElementsKind elements_kind,
+                                                    bool is_store) {
+  if (is_store) {
+    ASSERT(val != NULL);
+    if (elements_kind == FAST_DOUBLE_ELEMENTS) {
+      return new(zone()) HStoreKeyedFastDoubleElement(
+          elements, checked_key, val);
+    } else if (elements_kind == FAST_SMI_ONLY_ELEMENTS) {
+      AddInstruction(new(zone()) HCheckSmi(val));
+      return new(zone()) HStoreKeyedFastElement(
+          elements, checked_key, val, HStoreKeyedFastElement::VALUE_IS_SMI);
+    } else {
+      return new(zone()) HStoreKeyedFastElement(elements, checked_key, val);
+    }
+  } else {
+    if (elements_kind == FAST_DOUBLE_ELEMENTS) {
+      return new(zone()) HLoadKeyedFastDoubleElement(elements, checked_key);
+    } else {
+      return new(zone()) HLoadKeyedFastElement(elements, checked_key);

[danno, 2011/09/23 09:05:24]

If you're refactoring anyway, perhaps you might want to kill the arrow
anti-pattern?
http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

[Jakob Kummerow, 2011/09/26 11:30:32]

Done. Removed the unnecessary outer "else" and added comments to the other
"else" blocks.

+    }
+  }
+}
+
+
 HInstruction* HGraphBuilder::BuildMonomorphicElementAccess(HValue* object,
                                                            HValue* key,
                                                            HValue* val,
                                                            Expression* expr,
                                                            bool is_store) {
   ASSERT(expr->IsMonomorphic());
   Handle<Map> map = expr->GetMonomorphicReceiverType();
   AddInstruction(new(zone()) HCheckNonSmi(object));
   HInstruction* mapcheck = AddInstruction(new(zone()) HCheckMap(object, map));
-  if (!map->has_fast_elements() &&
-      !map->has_fast_double_elements() &&
+  bool fast_smi_only_elements = map->has_fast_smi_only_elements();
+  bool fast_elements = map->has_fast_elements();
+  bool fast_double_elements = map->has_fast_double_elements();
+  if (!fast_smi_only_elements &&
+      !fast_elements &&
+      !fast_double_elements &&
       !map->has_external_array_elements()) {
     return is_store ? BuildStoreKeyedGeneric(object, key, val)
                     : BuildLoadKeyedGeneric(object, key);
   }
   HInstruction* elements = AddInstruction(new(zone()) HLoadElements(object));
-  bool fast_double_elements = map->has_fast_double_elements();
-  if (is_store && map->has_fast_elements()) {
+  if (is_store && (fast_elements || fast_smi_only_elements)) {
     AddInstruction(new(zone()) HCheckMap(
         elements, isolate()->factory()->fixed_array_map()));
   }
   HInstruction* length = NULL;
   HInstruction* checked_key = NULL;
   if (map->has_external_array_elements()) {
     length = AddInstruction(new(zone()) HFixedArrayBaseLength(elements));
     checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
     HLoadExternalArrayPointer* external_elements =
         new(zone()) HLoadExternalArrayPointer(elements);
     AddInstruction(external_elements);
     return BuildExternalArrayElementAccess(external_elements, checked_key,
                                            val, map->elements_kind(), is_store);
   }
-  ASSERT(map->has_fast_elements() || fast_double_elements);
+  ASSERT(fast_smi_only_elements || fast_elements || fast_double_elements);

[danno, 2011/09/23 09:05:24]

Perhaps add predicates to the map that test for multiple values as we did for
Object's HasXXXXElements, like  fast_type_elements()?

[Jakob Kummerow, 2011/09/26 11:30:32]

I'd prefer not to do that, for two reasons:
(1) This ASSERT is only used once, so we wouldn't save any code by introducing a
predicate
(2) It's as much for documentation ("at this point in the code, X is handled")
as for asserting correctness, so I like to keep the listing of cases explicit to
the reader.

   if (map->instance_type() == JS_ARRAY_TYPE) {
     length = AddInstruction(new(zone()) HJSArrayLength(object, mapcheck));
   } else {
     length = AddInstruction(new(zone()) HFixedArrayBaseLength(elements));
   }
   checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
-  if (is_store) {
-    if (fast_double_elements) {
-      return new(zone()) HStoreKeyedFastDoubleElement(elements,
-                                                      checked_key,
-                                                      val);
-    } else {
-      return new(zone()) HStoreKeyedFastElement(elements, checked_key, val);
-    }
-  } else {
-    if (fast_double_elements) {
-      return new(zone()) HLoadKeyedFastDoubleElement(elements, checked_key);
-    } else {
-      return new(zone()) HLoadKeyedFastElement(elements, checked_key);
-    }
-  }
+  return BuildFastElementAccess(elements, checked_key, val,
+                                map->elements_kind(), is_store);
 }
 
 
 HValue* HGraphBuilder::HandlePolymorphicElementAccess(HValue* object,
                                                       HValue* key,
                                                       HValue* val,
                                                       Expression* prop,
                                                       int ast_id,
                                                       int position,
                                                       bool is_store,
@@ skipping 32 matching lines @@
   // FAST_DOUBLE_ELEMENTS and DICTIONARY_ELEMENTS are handled before external
   // arrays.
   STATIC_ASSERT(FAST_SMI_ONLY_ELEMENTS < FIRST_EXTERNAL_ARRAY_ELEMENTS_KIND);
   STATIC_ASSERT(FAST_ELEMENTS < FIRST_EXTERNAL_ARRAY_ELEMENTS_KIND);
   STATIC_ASSERT(FAST_DOUBLE_ELEMENTS < FIRST_EXTERNAL_ARRAY_ELEMENTS_KIND);
   STATIC_ASSERT(DICTIONARY_ELEMENTS < FIRST_EXTERNAL_ARRAY_ELEMENTS_KIND);
 
   for (ElementsKind elements_kind = FIRST_ELEMENTS_KIND;
        elements_kind <= LAST_ELEMENTS_KIND;
        elements_kind = ElementsKind(elements_kind + 1)) {
-    // After having handled FAST_ELEMENTS, FAST_SMI_ELEMENTS,
+    // After having handled FAST_ELEMENTS, FAST_SMI_ONLY_ELEMENTS,
     // FAST_DOUBLE_ELEMENTS and DICTIONARY_ELEMENTS, we need to add some code
     // that's executed for all external array cases.
     STATIC_ASSERT(LAST_EXTERNAL_ARRAY_ELEMENTS_KIND ==
                   LAST_ELEMENTS_KIND);
     if (elements_kind == FIRST_EXTERNAL_ARRAY_ELEMENTS_KIND
         && todo_external_array) {
       HInstruction* length =
           AddInstruction(new(zone()) HFixedArrayBaseLength(elements));
       checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
       external_elements = new(zone()) HLoadExternalArrayPointer(elements);
       AddInstruction(external_elements);
     }
     if (type_todo[elements_kind]) {
       HBasicBlock* if_true = graph()->CreateBasicBlock();
       HBasicBlock* if_false = graph()->CreateBasicBlock();
       elements_kind_branch = new(zone()) HCompareConstantEqAndBranch(
           elements_kind_instr, elements_kind, Token::EQ_STRICT);
       elements_kind_branch->SetSuccessorAt(0, if_true);
       elements_kind_branch->SetSuccessorAt(1, if_false);
       current_block()->Finish(elements_kind_branch);
 
       set_current_block(if_true);
       HInstruction* access;
       if (elements_kind == FAST_SMI_ONLY_ELEMENTS ||
           elements_kind == FAST_ELEMENTS ||
           elements_kind == FAST_DOUBLE_ELEMENTS) {
-        bool fast_double_elements =
-            elements_kind == FAST_DOUBLE_ELEMENTS;
-        if (is_store && !fast_double_elements) {
+        if (is_store && elements_kind == FAST_SMI_ONLY_ELEMENTS) {
+          // When the |object| has FAST_SMI_ONLY_ELEMENTS, chances are we'll

[danno, 2011/09/23 09:05:24]

stype nit: passive voice instead of "we"?

[Jakob Kummerow, 2011/09/26 11:30:32]

Done -- both comment and the code it referred to are obsolete, I have removed
them.

+          // only ever see more Smis being stored in it, so it's fine to
+          // just deopt if that assumption is ever wrong.
+          AddInstruction(new(zone()) HCheckSmi(val));
+        }
+        if (is_store && elements_kind != FAST_DOUBLE_ELEMENTS) {
           AddInstruction(new(zone()) HCheckMap(
               elements, isolate()->factory()->fixed_array_map(),
               elements_kind_branch));
         }
+        // TODO(jkummerow): We could get around the need for these two blocks

[danno, 2011/09/23 09:05:24]

stype nit: passive voice instead of "we"?

[Jakob Kummerow, 2011/09/26 11:30:32]

Done.

+        // in one of two ways:
+        // (1) Introduce ElementsKinds for JSArrays that are distinct from
+        //     those for fast objects.
+        // (2) Put the common instructions into a third "join" block. This
+        //     requires additional AST IDs that we can deopt to from inside
+        //     that join block. They must be added to the Property class (when
+        //     it's a keyed property) and registered in the full codegen.
         HBasicBlock* if_jsarray = graph()->CreateBasicBlock();
         HBasicBlock* if_fastobject = graph()->CreateBasicBlock();
         HHasInstanceTypeAndBranch* typecheck =
             new(zone()) HHasInstanceTypeAndBranch(object, JS_ARRAY_TYPE);
         typecheck->SetSuccessorAt(0, if_jsarray);
         typecheck->SetSuccessorAt(1, if_fastobject);
         current_block()->Finish(typecheck);
 
         set_current_block(if_jsarray);
         HInstruction* length;
-        if (is_store && elements_kind == FAST_SMI_ONLY_ELEMENTS) {
-          // For now, fall back to the generic stub for
-          // FAST_SMI_ONLY_ELEMENTS
-          access = AddInstruction(BuildStoreKeyedGeneric(object, key, val));
-        } else {
-          length = new(zone()) HJSArrayLength(object, typecheck);
-          AddInstruction(length);
-          checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
-          if (is_store) {
-            if (fast_double_elements) {
-              access = AddInstruction(
-                  new(zone()) HStoreKeyedFastDoubleElement(elements,
-                                                           checked_key,
-                                                           val));
-            } else {
-              access = AddInstruction(
-                  new(zone()) HStoreKeyedFastElement(elements,
-                                                     checked_key,
-                                                     val));
-            }
-          } else {
-            if (fast_double_elements) {
-              access = AddInstruction(
-                  new(zone()) HLoadKeyedFastDoubleElement(elements,
-                                                          checked_key));
-            } else {
-              access = AddInstruction(
-                  new(zone()) HLoadKeyedFastElement(elements, checked_key));
-            }
-            Push(access);
-          }
+        length = AddInstruction(new(zone()) HJSArrayLength(object, typecheck));
+        checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
+        access = AddInstruction(BuildFastElementAccess(
+            elements, checked_key, val, elements_kind, is_store));
+        if (!is_store) {
+          Push(access);
         }
 
         *has_side_effects |= access->HasSideEffects();
         if (position != -1) {
           access->set_position(position);
         }
         if_jsarray->Goto(join);
 
         set_current_block(if_fastobject);
         length = AddInstruction(new(zone()) HFixedArrayBaseLength(elements));
         checked_key = AddInstruction(new(zone()) HBoundsCheck(key, length));
-        if (is_store) {
-          if (fast_double_elements) {
-            access = AddInstruction(
-                new(zone()) HStoreKeyedFastDoubleElement(elements,
-                                                         checked_key,
-                                                         val));
-          } else {
-            access = AddInstruction(
-                new(zone()) HStoreKeyedFastElement(elements, checked_key, val));
-          }
-        } else {
-          if (fast_double_elements) {
-            access = AddInstruction(
-                new(zone()) HLoadKeyedFastDoubleElement(elements, checked_key));
-          } else {
-            access = AddInstruction(
-                new(zone()) HLoadKeyedFastElement(elements, checked_key));
-          }
-        }
+        access = AddInstruction(BuildFastElementAccess(
+            elements, checked_key, val, elements_kind, is_store));
       } else if (elements_kind == DICTIONARY_ELEMENTS) {
         if (is_store) {
           access = AddInstruction(BuildStoreKeyedGeneric(object, key, val));
         } else {
           access = AddInstruction(BuildLoadKeyedGeneric(object, key));
         }
       } else {  // External array elements.
         access = AddInstruction(BuildExternalArrayElementAccess(
             external_elements, checked_key, val, elements_kind, is_store));
       }
@@ skipping 2666 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify(false);  // No full verify.
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal