Make closures optimizable by Crankshaft compiler.

src/hydrogen.cc

 // Copyright 2010 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 2912 matching lines @@
 
 void HGraphBuilder::VisitVariableProxy(VariableProxy* expr) {
   Variable* variable = expr->AsVariable();
   if (variable == NULL) {
     BAILOUT("reference to rewritten variable");
   } else if (variable->IsStackAllocated()) {
     if (environment()->Lookup(variable)->CheckFlag(HValue::kIsArguments)) {
       BAILOUT("unsupported context for arguments object");
     }
     ast_context()->ReturnValue(environment()->Lookup(variable));
+  } else if (variable->AsSlot() != NULL &&
+             variable->AsSlot()->type() == Slot::CONTEXT) {

[Kevin Millikin (Chromium), 2010/12/20 12:05:44]

This predicate (->AsSlot() != NULL && ->AsSlot()->type == Slot::CONTEXT) should
probably be defined on the Variable class as something like "IsInContext()" or
"IsHeapAllocated()".

[antonm, 2010/12/20 20:39:24]

Done.

I named IsContextSlot as IMHO IsHeapAllocated rather leaks implementation detail
and IsInContext looks somewhat vague for me.  But if you prefer any other name,
just name it :)

+    if (variable->mode() == Variable::CONST) {
+      BAILOUT("reference to const context slot");
+    }
+    Slot* slot = variable->AsSlot();
+    CompilationInfo* info = graph()->info();
+    int context_chain_length = info->function()->scope()->
+        ContextChainLength(slot->var()->scope());
+    ASSERT(context_chain_length >= 0);
+    // TODO(antonm): if slot's value is not modified by closures, instead
+    // of reading it out of context, we could just embed the value as
+    // a constant.
+    HLoadContextSlot* instr =
+        new HLoadContextSlot(context_chain_length, slot->index());
+    ast_context()->ReturnInstruction(instr, expr->id());
   } else if (variable->is_global()) {
     LookupResult lookup;
     LookupGlobalPropertyCell(variable, &lookup, false);
     CHECK_BAILOUT;
 
     Handle<GlobalObject> global(graph()->info()->global_object());
     // TODO(3039103): Handle global property load through an IC call when access
     // checks are enabled.
     if (global->IsAccessCheckNeeded()) {
       BAILOUT("global object requires access check");
     }
     Handle<JSGlobalPropertyCell> cell(global->GetPropertyCell(&lookup));
     bool check_hole = !lookup.IsDontDelete() || lookup.IsReadOnly();
     HLoadGlobal* instr = new HLoadGlobal(cell, check_hole);
     ast_context()->ReturnInstruction(instr, expr->id());
   } else {
     BAILOUT("reference to non-stack-allocated/non-global variable");

[Kevin Millikin (Chromium), 2010/12/20 12:05:44]

You should probably change this bailout string.  All that's left is
Slot::LOOKUP, right?

[antonm, 2010/12/20 20:39:24]

Done.

Please check if wording is fine.

   }
 }
 
 
 void HGraphBuilder::VisitLiteral(Literal* expr) {
   HConstant* instr = new HConstant(expr->handle(), Representation::Tagged());
   ast_context()->ReturnInstruction(instr, expr->id());
 }
 
 
@@ skipping 477 matching lines @@
 
   if (expr->is_compound()) {
     HandleCompoundAssignment(expr);
     return;
   }
 
   if (var != NULL) {
     if (proxy->IsArguments()) BAILOUT("assignment to arguments");
 
     // Handle the assignment.
+    if (var->AsSlot() != NULL && var->AsSlot()->type() == Slot::CONTEXT) {

[Kevin Millikin (Chromium), 2010/12/20 12:05:44]

Instead of an early bailout, it would be better if this code should be
structured like the code in VisitVariableProxy, explicitly handling the cases:

if (/* global */) ...
else if (/* stack-allocated */) ...
else /* the rest, bailout */

[antonm, 2010/12/20 20:39:24]

Done.

+      BAILOUT("context slot assignment");
+    }
     if (var->is_global()) {
       VISIT_FOR_VALUE(expr->value());
       HandleGlobalVariableAssignment(var,
                                      Top(),
                                      expr->position(),
                                      expr->AssignmentId());
     } else {

[Kevin Millikin (Chromium), 2010/12/20 12:05:44]

Because here we don't want to have Slot::LOOKUP fall through the cracks.

[antonm, 2010/12/20 20:39:24]

Done.

       // We allow reference to the arguments object only in assignemtns
       // to local variables to make sure that the arguments object does
       // not escape and is not modified.
       VariableProxy* rhs = expr->value()->AsVariableProxy();
       if (rhs != NULL &&
           rhs->var()->IsStackAllocated() &&
           environment()->Lookup(rhs->var())->CheckFlag(HValue::kIsArguments)) {
         Push(environment()->Lookup(rhs->var()));
       } else {
         VISIT_FOR_VALUE(expr->value());
@@ skipping 2207 matching lines @@
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify();
   if (chunk_ != NULL) chunk_->Verify();
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal