Fix a bug in deoptimization after logical expressions in an effect context.

When deoptimizing to after an expression of the form (expr0 || expr1)
or (expr0 && expr1) in an effect context, the unoptimized code could
incorrectly see the value of the expression.

Handle the short-circuit binary operators specially in effect contexts.
This fixes the issue and will generate better code when the left
subexpression is boolean-valued.

BUG=v8:1166

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

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

src/hydrogen.cc

@@ -482,23 +482,6 @@ HConstant* HGraph::GetConstantFalse() {
 }
 
 
-void HSubgraph::AppendOptional(HSubgraph* graph,
-                               bool on_true_branch,
-                               HValue* value) {
-  ASSERT(HasExit() && graph->HasExit());
-  HBasicBlock* other_block = graph_->CreateBasicBlock();
-  HBasicBlock* join_block = graph_->CreateBasicBlock();
-
-  HTest* test = on_true_branch
-      ? new HTest(value, graph->entry_block(), other_block)
-      : new HTest(value, other_block, graph->entry_block());
-  exit_block_->Finish(test);
-  other_block->Goto(join_block);
-  graph->exit_block()->Goto(join_block);
-  exit_block_ = join_block;
-}
-
-
 void HSubgraph::AppendJoin(HSubgraph* then_graph,
                            HSubgraph* else_graph,
                            AstNode* node) {
@@ -4977,7 +4960,7 @@ void HGraphBuilder::VisitBinaryOperation(BinaryOperation* expr) {
       subgraph()->set_exit_block(eval_right);
       Visit(expr->right());
 
-    } else {
+    } else if (ast_context()->IsValue()) {
       VISIT_FOR_VALUE(expr->left());
       ASSERT(current_subgraph_->HasExit());
 
@@ -4987,9 +4970,50 @@ void HGraphBuilder::VisitBinaryOperation(BinaryOperation* expr) {
       HSubgraph* right_subgraph;
       right_subgraph = CreateBranchSubgraph(environment_copy);
       ADD_TO_SUBGRAPH(right_subgraph, expr->right());
-      current_subgraph_->AppendOptional(right_subgraph, is_logical_and, left);
-      current_subgraph_->exit_block()->SetJoinId(expr->id());
+
+      ASSERT(subgraph()->HasExit() && right_subgraph->HasExit());
+      // We need an extra block to maintain edge-split form.
+      HBasicBlock* empty_block = graph()->CreateBasicBlock();
+      HBasicBlock* join_block = graph()->CreateBasicBlock();
+
+      HTest* test = is_logical_and
+          ? new HTest(left, right_subgraph->entry_block(), empty_block)
+          : new HTest(left, empty_block, right_subgraph->entry_block());
+      subgraph()->exit_block()->Finish(test);
+      empty_block->Goto(join_block);
+      right_subgraph->exit_block()->Goto(join_block);
+      join_block->SetJoinId(expr->id());
+      subgraph()->set_exit_block(join_block);
       ast_context()->ReturnValue(Pop());
+    } else {
+      ASSERT(ast_context()->IsEffect());
+      // In an effect context, we don't need the value of the left
+      // subexpression, only its control flow and side effects.  We need an
+      // extra block to maintain edge-split form.
+      HBasicBlock* empty_block = graph()->CreateBasicBlock();
+      HBasicBlock* right_block = graph()->CreateBasicBlock();
+      HBasicBlock* join_block = graph()->CreateBasicBlock();
+      if (is_logical_and) {
+        VISIT_FOR_CONTROL(expr->left(), right_block, empty_block);
+      } else {
+        VISIT_FOR_CONTROL(expr->left(), empty_block, right_block);
+      }
+      // TODO(kmillikin): Find a way to fix this.  It's ugly that there are
+      // actually two empty blocks (one here and one inserted by
+      // TestContext::BuildBranch, and that they both have an HSimulate
+      // though the second one is not a merge node, and that we really have
+      // no good AST ID to put on that first HSimulate.
+      empty_block->SetJoinId(expr->id());
+      right_block->SetJoinId(expr->RightId());
+      subgraph()->set_exit_block(right_block);
+      VISIT_FOR_EFFECT(expr->right());
+
+      empty_block->Goto(join_block);
+      subgraph()->exit_block()->Goto(join_block);
+      join_block->SetJoinId(expr->id());
+      subgraph()->set_exit_block(join_block);
+      // We did not materialize any value in the predecessor environments,
+      // so there is no need to handle it here.
     }
 
   } else {

src/hydrogen.h

@@ -210,9 +210,6 @@ class HSubgraph: public ZoneObject {
 
   void PreProcessOsrEntry(IterationStatement* statement);
 
-  void AppendOptional(HSubgraph* graph,
-                      bool on_true_branch,
-                      HValue* boolean_value);
   void AppendJoin(HSubgraph* then_graph, HSubgraph* else_graph, AstNode* node);
   void AppendWhile(HSubgraph* condition,
                    HSubgraph* body,

test/mjsunit/regress/regress-1166.js

+// 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
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Deoptimization after a short-circuit logical operation in an effect
+// context should not see the value of the expression.
+function observe(x, y) { return x; }
+
+function test(x) { return observe(1, ((false || false), x + 1)); }
+
+for (var i = 0; i < 10000000; ++i) test(0);
+test("a");