In the toplevel compiler, shift the responsibility of assigning a...

src/compiler.cc

Index: src/compiler.cc
===================================================================
--- src/compiler.cc	(revision 3152)
+++ src/compiler.cc	(working copy)
@@ -46,7 +46,10 @@
  public:
   enum CodeGenTag { NORMAL, FAST };
 
-  CodeGenSelector() : has_supported_syntax_(true) {}
+  CodeGenSelector()
+      : has_supported_syntax_(true),
+        location_(Location::Nowhere()) {

[fschneider, 2009/10/28 10:06:01]

I'm not sure: Should this match the default location for ast nodes(TEMP)?

[Kevin Millikin (Chromium), 2009/10/28 11:39:38]

Possibly.  I wanted it to be nowhere when not inside an expression (visiting a
statement or declaration).  Now that every AST node gets a location explicitly
assigned, it might make sense to introduce an INVALID or UNITIALIZED one as
expression default to catch failure to assign.

I would do that as a separate change.

+  }
 
   CodeGenTag Select(FunctionLiteral* fun);
 
@@ -54,6 +57,14 @@
   void VisitDeclarations(ZoneList<Declaration*>* decls);
   void VisitStatements(ZoneList<Statement*>* stmts);
 
+  // Visit an expression in effect context with a desired location of
+  // nowhere.
+  void VisitAsEffect(Expression* expr);
+
+  // Visit an expression in value context with a desired location of
+  // temporary.
+  void VisitAsValue(Expression* expr);
+
   // AST node visit functions.
 #define DECLARE_VISIT(type) virtual void Visit##type(type* node);
   AST_NODE_LIST(DECLARE_VISIT)
@@ -61,6 +72,9 @@
 
   bool has_supported_syntax_;
 
+  // The desired location of the currently visited expression.
+  Location location_;
+
   DISALLOW_COPY_AND_ASSIGN(CodeGenSelector);
 };
 
@@ -499,6 +513,30 @@
 }
 
 
+void CodeGenSelector::VisitAsEffect(Expression* expr) {
+  if (location_.is_nowhere()) {
+    Visit(expr);
+  } else {
+    Location saved = location_;
+    location_ = Location::Nowhere();
+    Visit(expr);
+    location_ = saved;
+  }
+}
+
+
+void CodeGenSelector::VisitAsValue(Expression* expr) {
+  if (location_.is_temporary()) {
+    Visit(expr);
+  } else {
+    Location saved = location_;
+    location_ = Location::Temporary();
+    Visit(expr);
+    location_ = saved;
+  }
+}
+
+
 void CodeGenSelector::VisitDeclaration(Declaration* decl) {
   Variable* var = decl->proxy()->var();
   if (!var->is_global() || var->mode() == Variable::CONST) {
@@ -513,10 +551,7 @@
 
 
 void CodeGenSelector::VisitExpressionStatement(ExpressionStatement* stmt) {
-  Expression* expr = stmt->expression();
-  Visit(expr);
-  CHECK_BAILOUT;
-  expr->set_location(Location::Nowhere());
+  VisitAsEffect(stmt->expression());
 }
 
 
@@ -541,7 +576,7 @@
 
 
 void CodeGenSelector::VisitReturnStatement(ReturnStatement* stmt) {
-  Visit(stmt->expression());
+  VisitAsValue(stmt->expression());
 }
 
 
@@ -599,6 +634,7 @@
   if (!expr->AllowsLazyCompilation()) {
     BAILOUT("FunctionLiteral does not allow lazy compilation");
   }
+  expr->set_location(location_);
 }
 
 
@@ -614,37 +650,72 @@
 
 
 void CodeGenSelector::VisitSlot(Slot* expr) {
-  Slot::Type type = expr->type();
-  if (type != Slot::PARAMETER && type != Slot::LOCAL) {
-    BAILOUT("non-parameter/non-local slot reference");
-  }
+  UNREACHABLE();
 }
 
 
 void CodeGenSelector::VisitVariableProxy(VariableProxy* expr) {
   Expression* rewrite = expr->var()->rewrite();
-  if (rewrite != NULL) Visit(rewrite);
+  // A rewrite of NULL indicates a global variable.
+  if (rewrite != NULL) {
+    Slot* slot = rewrite->AsSlot();
+    if (slot != NULL) {
+      Slot::Type type = slot->type();
+      if (type != Slot::PARAMETER && type != Slot::LOCAL) {
+        BAILOUT("non-parameter/non-local slot reference");
+      }

[William Hesse, 2009/10/28 10:28:54]

Control flow is tricky here.  Reversing sense of slot != NULL makes this an if,
else if that is easier to read.

[Kevin Millikin (Chromium), 2009/10/28 11:39:38]

I see what you mean.  I've changed it.

+    } else {
+      BAILOUT("non-global/non-slot variable reference");
+    }
+  }
+  expr->set_location(location_);
 }
 
 
 void CodeGenSelector::VisitLiteral(Literal* expr) {
-  // Literals are supported.
+  expr->set_location(location_);
 }
 
 
 void CodeGenSelector::VisitRegExpLiteral(RegExpLiteral* expr) {
-  // RegexpLiterals are supported.
+  expr->set_location(location_);
 }
 
 
 void CodeGenSelector::VisitObjectLiteral(ObjectLiteral* expr) {
-  for (int i = 0; i < expr->properties()->length(); i++) {
-    ObjectLiteral::Property* property = expr->properties()->at(i);
-    Visit(property->key());
+  ZoneList<ObjectLiteral::Property*>* properties = expr->properties();
+
+  for (int i = 0, len = properties->length(); i < len; i++) {
+    ObjectLiteral::Property* property = properties->at(i);
+    if (property->IsCompileTimeValue()) continue;
+
+    switch (property->kind()) {
+      case ObjectLiteral::Property::CONSTANT:
+        UNREACHABLE();
+
+      // For (non-compile-time) materialized literals and computed
+      // properties with symbolic keys we will use an IC and therefore not
+      // generate code for the key.
+      case ObjectLiteral::Property::COMPUTED:  // Fall through.
+      case ObjectLiteral::Property::MATERIALIZED_LITERAL:
+        if (property->key()->handle()->IsSymbol()) {
+          break;
+        }
+        // Fall through.
+
+      // In all other cases we need the key's value on the stack
+      // for a runtime call.  (Relies on TEMP meaning STACK.)
+      case ObjectLiteral::Property::GETTER:  // Fall through.
+      case ObjectLiteral::Property::SETTER:  // Fall through.
+      case ObjectLiteral::Property::PROTOTYPE:
+        VisitAsValue(property->key());
+        CHECK_BAILOUT;
+        break;
+    }
+    VisitAsValue(property->value());
     CHECK_BAILOUT;
-    Visit(property->value());
-    CHECK_BAILOUT;
   }
+  expr->set_location(location_);
 }
 
 
@@ -654,9 +725,10 @@
     Expression* subexpr = subexprs->at(i);
     if (subexpr->AsLiteral() != NULL) continue;
     if (CompileTimeValue::IsCompileTimeValue(subexpr)) continue;
-    Visit(subexpr);
+    VisitAsValue(subexpr);
     CHECK_BAILOUT;
   }
+  expr->set_location(location_);
 }
 
 
@@ -668,7 +740,10 @@
 void CodeGenSelector::VisitAssignment(Assignment* expr) {
   // We support plain non-compound assignments to parameters and
   // non-context (stack-allocated) locals.
-  if (expr->starts_initialization_block()) BAILOUT("initialization block");
+  if (expr->starts_initialization_block() ||
+      expr->ends_initialization_block()) {
+    BAILOUT("initialization block start");
+  }
 
   Token::Value op = expr->op();
   if (op == Token::INIT_CONST) BAILOUT("initialize constant");
@@ -687,7 +762,8 @@
     }
   }
 
-  Visit(expr->value());
+  VisitAsValue(expr->value());
+  expr->set_location(location_);
 }
 
 
@@ -718,11 +794,12 @@
   } else {
     BAILOUT("Call to a non-global function");
   }
-  // Check all arguments to the call
+  // Check all arguments to the call.  (Relies on TEMP meaning STACK.)
   for (int i = 0; i < args->length(); i++) {
-    Visit(args->at(i));
+    VisitAsValue(args->at(i));
     CHECK_BAILOUT;
   }
+  expr->set_location(location_);
 }
 
 
@@ -740,16 +817,18 @@
 
 void CodeGenSelector::VisitCallRuntime(CallRuntime* expr) {
   // In case of JS runtime function bail out.
-  if (expr->function() == NULL) BAILOUT("CallRuntime");
+  if (expr->function() == NULL) BAILOUT("call JS runtime function");
   // Check for inline runtime call
   if (expr->name()->Get(0) == '_' &&
       CodeGenerator::FindInlineRuntimeLUT(expr->name()) != NULL) {
-    BAILOUT("InlineRuntimeCall");
+    BAILOUT("inlined runtime call");
   }
+  // Check all arguments to the call.  (Relies on TEMP meaning STACK.)
   for (int i = 0; i < expr->arguments()->length(); i++) {
-    Visit(expr->arguments()->at(i));
+    VisitAsValue(expr->arguments()->at(i));
     CHECK_BAILOUT;
   }
+  expr->set_location(location_);
 }
 
 
@@ -766,14 +845,17 @@
 void CodeGenSelector::VisitBinaryOperation(BinaryOperation* expr) {
   switch (expr->op()) {
     case Token::OR:
-      Visit(expr->left());
+      VisitAsValue(expr->left());
       CHECK_BAILOUT;
+      // The location for the right subexpression is the same as for the
+      // whole expression.
       Visit(expr->right());
       break;
 
     default:
       BAILOUT("Unsupported binary operation");
   }
+  expr->set_location(location_);
 }