Reenable the duplicated test at the bottom of for and while loops. It

is enabled for a loop by the absence of function literal
subexpressions in the condition expression.

Review URL: http://codereview.chromium.org/42127

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1503 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/ast.h

@@ -291,8 +291,13 @@ class LoopStatement: public IterationStatement {
   enum Type { DO_LOOP, FOR_LOOP, WHILE_LOOP };
 
   LoopStatement(ZoneStringList* labels, Type type)
-      : IterationStatement(labels), type_(type), init_(NULL),
-        cond_(NULL), next_(NULL) { }
+      : IterationStatement(labels),
+        type_(type),
+        init_(NULL),
+        cond_(NULL),
+        next_(NULL),
+        has_function_literal_(false) {
+  }
 
   void Initialize(Statement* init,
                   Expression* cond,
@@ -312,6 +317,7 @@ class LoopStatement: public IterationStatement {
   Statement* init() const  { return init_; }
   Expression* cond() const  { return cond_; }
   Statement* next() const  { return next_; }
+  bool has_function_literal() const { return has_function_literal_; }
 
 #ifdef DEBUG
   const char* OperatorString() const;
@@ -322,6 +328,10 @@ class LoopStatement: public IterationStatement {
   Statement* init_;
   Expression* cond_;
   Statement* next_;
+  // True if there is a function literal subexpression in the condition.
+  bool has_function_literal_;
+
+  friend class AstOptimizer;
 };
 
 

src/codegen-ia32.cc

@@ -2228,18 +2228,24 @@ void CodeGenerator::VisitLoopStatement(LoopStatement* node) {
     }
 
     case LoopStatement::WHILE_LOOP: {
-      // TODO(260): This flag controls whether to duplicate the test
-      // at the bottom of the loop.  Replace it with a better
-      // indication of when it is safe to do so.
-      static const bool test_at_bottom = false;
+      // Do not duplicate conditions with function literal
+      // subexpressions.  This can cause us to compile the function
+      // literal twice.
+      bool test_at_bottom = !node->has_function_literal();
 
-      JumpTarget body(this);  // Initialized as forward-only.
       IncrementLoopNesting();
 
       // If the condition is always false and has no side effects, we
       // do not need to compile anything.
       if (info == ALWAYS_FALSE) break;
 
+      JumpTarget body;
+      if (test_at_bottom) {
+        body.Initialize(this, JumpTarget::BIDIRECTIONAL);
+      } else {
+        body.Initialize(this);
+      }
+
       // Based on the condition analysis, compile the test as necessary.
       if (info == ALWAYS_TRUE) {
         // We will not compile the test expression.  Label the top of
@@ -2252,7 +2258,6 @@ void CodeGenerator::VisitLoopStatement(LoopStatement* node) {
           // Continue is the test at the bottom, no need to label the
           // test at the top.  The body is a backward target.
           node->continue_target()->Initialize(this);
-          body.make_bidirectional();
         } else {
           // Label the test at the top as the continue target.  The
           // body is a forward-only target.
@@ -2321,13 +2326,10 @@ void CodeGenerator::VisitLoopStatement(LoopStatement* node) {
     }
 
     case LoopStatement::FOR_LOOP: {
-      // TODO(260): This flag controls whether to duplicate the test
-      // at the bottom of the loop.  Replace it with a better
-      // indication of when it is safe to do so.
-      static const bool test_at_bottom = false;
-
-      JumpTarget loop(this, JumpTarget::BIDIRECTIONAL);
-      JumpTarget body(this);
+      // Do not duplicate conditions with function literal
+      // subexpressions.  This can cause us to compile the function
+      // literal twice.
+      bool test_at_bottom = !node->has_function_literal();
 
       // Compile the init expression if present.
       if (node->init() != NULL) {
@@ -2340,6 +2342,19 @@ void CodeGenerator::VisitLoopStatement(LoopStatement* node) {
       // do not need to compile anything else.
       if (info == ALWAYS_FALSE) break;
 
+      // Target for backward edge if no test at the bottom, otherwise
+      // unused.
+      JumpTarget loop(this, JumpTarget::BIDIRECTIONAL);
+
+      // Target for backward edge if there is a test at the bottom,
+      // otherwise used as target for test at the top.
+      JumpTarget body;
+      if (test_at_bottom) {
+        body.Initialize(this, JumpTarget::BIDIRECTIONAL);
+      } else {
+        body.Initialize(this);
+      }
+
       // Based on the condition analysis, compile the test as necessary.
       if (info == ALWAYS_TRUE) {
         // We will not compile the test expression.  Label the top of

src/jump-target.h

@@ -93,8 +93,6 @@ class JumpTarget : public Malloced {  // Shadows are dynamically allocated.
     entry_frame_ = frame;
   }
 
-  void make_bidirectional() { direction_ = BIDIRECTIONAL; }
-
   // Predicates testing the state of the encapsulated label.
   bool is_bound() const { return is_bound_; }
   bool is_linked() const { return is_linked_; }

src/rewriter.cc

@@ -42,6 +42,10 @@ class AstOptimizer: public AstVisitor {
   void Optimize(ZoneList<Statement*>* statements);
 
  private:
+  // Used for loop condition analysis.  Cleared before visiting a loop
+  // condition, set when a function literal is visited.
+  bool has_function_literal_;
+
   // Helpers
   void OptimizeArguments(ZoneList<Expression*>* arguments);
 
@@ -89,14 +93,14 @@ void AstOptimizer::VisitIfStatement(IfStatement* node) {
 }
 
 
-
-
 void AstOptimizer::VisitLoopStatement(LoopStatement* node) {
   if (node->init() != NULL) {
     Visit(node->init());
   }
   if (node->cond() != NULL) {
+    has_function_literal_ = false;
     Visit(node->cond());
+    node->has_function_literal_ = has_function_literal_;
   }
   if (node->body() != NULL) {
     Visit(node->body());
@@ -182,6 +186,7 @@ void AstOptimizer::VisitDebuggerStatement(DebuggerStatement* node) {
 
 void AstOptimizer::VisitFunctionLiteral(FunctionLiteral* node) {
   USE(node);
+  has_function_literal_ = true;
 }