Add optimized compiler support for generic global loads....

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 2095 matching lines @@
   // TODO(kasperl): Try to improve the way we compute the last added
   // instruction. The NULL check makes me uncomfortable.
   HValue* last = subgraph()->exit_block()->GetLastInstruction();
   if (last != NULL && last->HasSideEffects()) {
     AddSimulate(expr->id());
   }
   ASSERT(environment()->values()->length() == original_height + 1);
 }
 
 
+void HGraphBuilder::VisitForTypeofValue(Expression* expr) {

[Kevin Millikin (Chromium), 2010/12/09 13:16:51]

It seems to me that "parent is typeof" is just a flag on a value context, so
this function could be:

void HGraphBuilder::VisitForTypeofValue(Expression* expr) {
  ValueContext for_value(this);
  for_value.mark_as_typeof();
  Visit(expr);
}

That also dodges any problems with "HandleGlobalVariableLoad" (see below).

[fschneider, 2010/12/22 16:02:34]

Done.

+  VariableProxy* proxy = expr->AsVariableProxy();
+  if (proxy != NULL && !proxy->var()->is_this() && proxy->var()->is_global()) {

[Kevin Millikin (Chromium), 2010/12/09 13:16:51]

I think you can just ask the proxy is_this (and maybe is_global?).  If you
follow my suggestion above you don't need to do anything here, though.

[fschneider, 2010/12/22 16:02:34]

Done. This part of the code is now gone.

+#ifdef DEBUG
+    int original_height = environment()->values()->length();
+#endif
+    HandleGlobalVariableLoad(proxy, true);  // Inside typeof.
+    // Generic global loads always have a side effect.
+    AddSimulate(expr->id());
+    ASSERT(environment()->values()->length() == original_height + 1);
+  } else {
+    VisitForValue(expr);
+  }
+}
+
+
 HValue* HGraphBuilder::VisitArgument(Expression* expr) {
   VisitForValue(expr);
   if (HasStackOverflow() || !subgraph()->HasExit()) return NULL;
   return environment()->Top();
 }
 
 
 void HGraphBuilder::VisitArgumentList(ZoneList<Expression*>* arguments) {
   for (int i = 0; i < arguments->length(); i++) {
     VisitArgument(arguments->at(i));
@@ skipping 805 matching lines @@
                  then_graph->entry_block(),
                  else_graph->entry_block(),
                  false, false);
   if (HasStackOverflow()) return;
   ADD_TO_SUBGRAPH(then_graph, expr->then_expression());
   ADD_TO_SUBGRAPH(else_graph, expr->else_expression());
   current_subgraph_->AppendJoin(then_graph, else_graph, expr);
 }
 
 
 void HGraphBuilder::LookupGlobalPropertyCell(VariableProxy* expr,

[Kevin Millikin (Chromium), 2010/12/09 13:16:51]

It seems like it would be cleaner for this function to return a boolean
indicating whether it was found or not, rather than taking a pointer to a flag.

[fschneider, 2010/12/22 16:02:34]

I wanted to be able to use the BAILOUT macro from inside, but maybe it's not
worth it.

                                              LookupResult* lookup,
-                                             bool is_store) {
+                                             bool is_store,
+                                             bool* is_generic) {
+  *is_generic = true;
   if (expr->is_this()) {
     BAILOUT("global this reference");
   }
   if (!graph()->info()->has_global_object()) {
     BAILOUT("no global object to optimize VariableProxy");
   }
   Handle<GlobalObject> global(graph()->info()->global_object());
   global->Lookup(*expr->name(), lookup);
-  if (!lookup->IsProperty()) {
-    BAILOUT("global variable cell not yet introduced");
+  if (!lookup->IsProperty()) return;
+  if (lookup->type() != NORMAL) return;
+  if (is_store && lookup->IsReadOnly()) {
+    BAILOUT("store to read-only global variable");
   }
-  if (lookup->type() != NORMAL) {
-    BAILOUT("global variable has accessors");
-  }
-  if (is_store && lookup->IsReadOnly()) {
-    BAILOUT("read-only global variable");
+  *is_generic = false;
+}
+
+
+void HGraphBuilder::HandleGlobalVariableLoad(VariableProxy* expr,

[Kevin Millikin (Chromium), 2010/12/09 13:16:51]

We need to restore some sanity to these helper functions.  There are dozens of
"Visit....", "Build....", and "Handle...." in here with no consistent naming
scheme.

I actually inlined this function and removed its definition in my last change
because I was about to put a comment to the effect of "This function should only
be called in tail position as part of visiting...." and just decided to get rid
of the (called once) helper.

[fschneider, 2010/12/22 16:02:34]

Done.

+                                             bool inside_typeof) {
+  LookupResult lookup;
+  bool is_generic = true;
+  LookupGlobalPropertyCell(expr, &lookup, false, &is_generic);
+  CHECK_BAILOUT;
+
+  Handle<GlobalObject> global(graph()->info()->global_object());
+  if (is_generic || global->IsAccessCheckNeeded()) {
+    PushAndAdd(new HLoadGlobalGeneric(expr->var()->name(), inside_typeof));

[Kevin Millikin (Chromium), 2010/12/09 13:16:51]

Here you could just ask the ast_context if it is a typeof context, which is only
true for value contexts with the typeof bit set.  There's no need to even pass
it to this helper if you set up the context of the subexpression as suggested
above.

[fschneider, 2010/12/22 16:02:34]

Done.

+  } else {
+    Handle<JSGlobalPropertyCell> cell(global->GetPropertyCell(&lookup));
+    bool check_hole = !lookup.IsDontDelete() || lookup.IsReadOnly();
+    PushAndAdd(new HLoadGlobal(cell, check_hole));
   }
 }
 
 
-void HGraphBuilder::HandleGlobalVariableLoad(VariableProxy* expr) {
-  LookupResult lookup;
-  LookupGlobalPropertyCell(expr, &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();
-  PushAndAdd(new HLoadGlobal(cell, check_hole));
-}
-
-
 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");
     }
     Push(environment()->Lookup(variable));
   } else if (variable->is_global()) {
-    HandleGlobalVariableLoad(expr);
+    HandleGlobalVariableLoad(expr, false);  // Not inside typeof.
   } else {
     BAILOUT("reference to non-stack-allocated/non-global variable");
   }
 }
 
 
 void HGraphBuilder::VisitLiteral(Literal* expr) {
   PushAndAdd(new HConstant(expr->handle(), Representation::Tagged()));
 }
 
@@ skipping 331 matching lines @@
   Push(value);
   instr->set_position(expr->position());
   AddInstruction(instr);
 }
 
 
 void HGraphBuilder::HandleGlobalVariableAssignment(VariableProxy* proxy,
                                                    HValue* value,
                                                    int position) {
   LookupResult lookup;
-  LookupGlobalPropertyCell(proxy, &lookup, true);
+  bool is_generic;
+  LookupGlobalPropertyCell(proxy, &lookup, true, &is_generic);
   CHECK_BAILOUT;
 
   Handle<GlobalObject> global(graph()->info()->global_object());
-  Handle<JSGlobalPropertyCell> cell(global->GetPropertyCell(&lookup));
-  HInstruction* instr = new HStoreGlobal(value, cell);
-  instr->set_position(position);
-  AddInstruction(instr);
+  if (!is_generic && !global->IsAccessCheckNeeded()) {
+    Handle<JSGlobalPropertyCell> cell(global->GetPropertyCell(&lookup));
+    HInstruction* instr = new HStoreGlobal(value, cell);
+    instr->set_position(position);
+    AddInstruction(instr);
+  } else {
+    BAILOUT("unsupported store to global");
+  }
 }
 
 
 void HGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
   Expression* target = expr->target();
   VariableProxy* proxy = target->AsVariableProxy();
   Variable* var = proxy->AsVariable();
   Property* prop = target->AsProperty();
   ASSERT(var == NULL || prop == NULL);
 
@@ skipping 1032 matching lines @@
         break;
       case Token::SUB:
         instr = new HMul(graph_->GetConstantMinus1(), value);
         break;
       default:
         UNREACHABLE();
         break;
     }
     PushAndAdd(instr);
   } else if (op == Token::TYPEOF) {
-    VISIT_FOR_VALUE(expr->expression());
+    VisitForTypeofValue(expr->expression());
+    if (HasStackOverflow()) return;
     HValue* value = Pop();
     PushAndAdd(new HTypeof(value));
   } else {
     BAILOUT("Value: unsupported unary operation");
   }
 }
 
 
 void HGraphBuilder::VisitIncrementOperation(IncrementOperation* expr) {
   // IncrementOperation is never visited by the visitor. It only
@@ skipping 280 matching lines @@
     PushAndAdd(instr, expr->position());
     return;
   }
 
   // Check for the pattern: typeof <expression> == <string literal>.
   UnaryOperation* left_unary = expr->left()->AsUnaryOperation();
   Literal* right_literal = expr->right()->AsLiteral();
   if ((expr->op() == Token::EQ || expr->op() == Token::EQ_STRICT) &&
       left_unary != NULL && left_unary->op() == Token::TYPEOF &&
       right_literal != NULL && right_literal->handle()->IsString()) {
-    VISIT_FOR_VALUE(left_unary->expression());
+    VisitForTypeofValue(left_unary->expression());
+    if (HasStackOverflow()) return;
     HValue* left = Pop();
     HInstruction* instr = new HTypeofIs(left,
         Handle<String>::cast(right_literal->handle()));
     PushAndAdd(instr, expr->position());
     return;
   }
 
   VISIT_FOR_VALUE(expr->left());
   VISIT_FOR_VALUE(expr->right());
 
@@ skipping 826 matching lines @@
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify();
   if (chunk_ != NULL) chunk_->Verify();
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal