[ignition] Support n-ary logical ops

Following up on adding n-ary nodes, this extends the parser and
interpreter to support n-ary logical operations.

Bug: v8:6964
Bug: chromium:731861
Change-Id: Ife2141c389b9abccd917ab2aaddf399c436ef777
Reviewed-on: https://chromium-review.googlesource.com/735497Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49029}

src/interpreter/bytecode-generator.cc

@@ -4008,31 +4008,82 @@ void BytecodeGenerator::VisitNaryCommaExpression(NaryOperation* expr) {
   Visit(expr->subsequent(expr->subsequent_length() - 1));
 }
 
-void BytecodeGenerator::BuildLogicalTest(Token::Value token, Expression* left,
+void BytecodeGenerator::VisitLogicalTestSubExpression(
+    Token::Value token, Expression* expr, BytecodeLabels* then_labels,
+    BytecodeLabels* else_labels) {
+  DCHECK(token == Token::OR || token == Token::AND);
+
+  BytecodeLabels test_next(zone());
+  if (token == Token::OR) {
+    VisitForTest(expr, then_labels, &test_next, TestFallthrough::kElse);
+  } else {
+    DCHECK_EQ(Token::AND, token);
+    VisitForTest(expr, &test_next, else_labels, TestFallthrough::kThen);
+  }
+  test_next.Bind(builder());
+}
+
+void BytecodeGenerator::VisitLogicalTest(Token::Value token, Expression* left,
                                          Expression* right) {
   DCHECK(token == Token::OR || token == Token::AND);
   TestResultScope* test_result = execution_result()->AsTest();
   BytecodeLabels* then_labels = test_result->then_labels();
   BytecodeLabels* else_labels = test_result->else_labels();
   TestFallthrough fallthrough = test_result->fallthrough();
-  {
-    // Visit the left side using current TestResultScope.
-    BytecodeLabels test_right(zone());
-    if (token == Token::OR) {
-      test_result->set_fallthrough(TestFallthrough::kElse);
-      test_result->set_else_labels(&test_right);
-    } else {
-      DCHECK_EQ(Token::AND, token);
-      test_result->set_fallthrough(TestFallthrough::kThen);
-      test_result->set_then_labels(&test_right);
-    }
-    VisitInSameTestExecutionScope(left);
-    test_right.Bind(builder());
-  }
-  // Visit the right side in a new TestResultScope.
+
+  VisitLogicalTestSubExpression(token, left, then_labels, else_labels);
+  // The last test has the same then, else and fallthrough as the parent test.
   VisitForTest(right, then_labels, else_labels, fallthrough);
 }
 
+void BytecodeGenerator::VisitNaryLogicalTest(Token::Value token,
+                                             NaryOperation* expr) {
+  DCHECK(token == Token::OR || token == Token::AND);
+  DCHECK_GT(expr->subsequent_length(), 0);
+
+  TestResultScope* test_result = execution_result()->AsTest();
+  BytecodeLabels* then_labels = test_result->then_labels();
+  BytecodeLabels* else_labels = test_result->else_labels();
+  TestFallthrough fallthrough = test_result->fallthrough();
+
+  VisitLogicalTestSubExpression(token, expr->first(), then_labels, else_labels);
+  for (size_t i = 0; i < expr->subsequent_length() - 1; ++i) {
+    VisitLogicalTestSubExpression(token, expr->subsequent(i), then_labels,
+                                  else_labels);
+  }
+  // The last test has the same then, else and fallthrough as the parent test.
+  VisitForTest(expr->subsequent(expr->subsequent_length() - 1), then_labels,
+               else_labels, fallthrough);
+}
+
+bool BytecodeGenerator::VisitLogicalOrSubExpression(
+    Expression* expr, BytecodeLabels* end_labels) {
+  if (expr->ToBooleanIsTrue()) {
+    VisitForAccumulatorValue(expr);
+    end_labels->Bind(builder());
+    return true;
+  } else if (!expr->ToBooleanIsFalse()) {
+    TypeHint type_hint = VisitForAccumulatorValue(expr);
+    builder()->JumpIfTrue(ToBooleanModeFromTypeHint(type_hint),
+                          end_labels->New());
+  }
+  return false;
+}
+
+bool BytecodeGenerator::VisitLogicalAndSubExpression(
+    Expression* expr, BytecodeLabels* end_labels) {
+  if (expr->ToBooleanIsFalse()) {
+    VisitForAccumulatorValue(expr);
+    end_labels->Bind(builder());
+    return true;
+  } else if (!expr->ToBooleanIsTrue()) {
+    TypeHint type_hint = VisitForAccumulatorValue(expr);
+    builder()->JumpIfFalse(ToBooleanModeFromTypeHint(type_hint),
+                           end_labels->New());
+  }
+  return false;
+}
+
 void BytecodeGenerator::VisitLogicalOrExpression(BinaryOperation* binop) {
   Expression* left = binop->left();
   Expression* right = binop->right();
@@ -4044,26 +4095,42 @@ void BytecodeGenerator::VisitLogicalOrExpression(BinaryOperation* binop) {
     } else if (left->ToBooleanIsFalse() && right->ToBooleanIsFalse()) {
       builder()->Jump(test_result->NewElseLabel());
     } else {
-      BuildLogicalTest(Token::OR, left, right);
+      VisitLogicalTest(Token::OR, left, right);
     }
     test_result->SetResultConsumedByTest();
   } else {
-    if (left->ToBooleanIsTrue()) {
-      VisitForAccumulatorValue(left);
-    } else if (left->ToBooleanIsFalse()) {
-      VisitForAccumulatorValue(right);
-    } else {
-      BytecodeLabel end_label;
-      TypeHint type_hint = VisitForAccumulatorValue(left);
-      builder()->JumpIfTrue(ToBooleanModeFromTypeHint(type_hint), &end_label);
-      VisitForAccumulatorValue(right);
-      builder()->Bind(&end_label);
-    }
+    BytecodeLabels end_labels(zone());
+    if (VisitLogicalOrSubExpression(left, &end_labels)) return;
+    VisitForAccumulatorValue(right);
+    end_labels.Bind(builder());
   }
 }
+
 void BytecodeGenerator::VisitNaryLogicalOrExpression(NaryOperation* expr) {
-  // TODO(leszeks): Implement.
-  UNREACHABLE();
+  Expression* first = expr->first();
+  DCHECK_GT(expr->subsequent_length(), 0);
+
+  if (execution_result()->IsTest()) {
+    TestResultScope* test_result = execution_result()->AsTest();
+    if (first->ToBooleanIsTrue()) {
+      builder()->Jump(test_result->NewThenLabel());
+    } else {
+      VisitNaryLogicalTest(Token::OR, expr);
+    }
+    test_result->SetResultConsumedByTest();
+  } else {
+    BytecodeLabels end_labels(zone());
+    if (VisitLogicalOrSubExpression(first, &end_labels)) return;
+    for (size_t i = 0; i < expr->subsequent_length() - 1; ++i) {
+      if (VisitLogicalOrSubExpression(expr->subsequent(i), &end_labels)) {
+        return;
+      }
+    }
+    // We have to visit the last value even if it's true, because we need its
+    // actual value.
+    VisitForAccumulatorValue(expr->subsequent(expr->subsequent_length() - 1));
+    end_labels.Bind(builder());
+  }
 }
 
 void BytecodeGenerator::VisitLogicalAndExpression(BinaryOperation* binop) {
@@ -4077,26 +4144,42 @@ void BytecodeGenerator::VisitLogicalAndExpression(BinaryOperation* binop) {
     } else if (left->ToBooleanIsTrue() && right->ToBooleanIsTrue()) {
       builder()->Jump(test_result->NewThenLabel());
     } else {
-      BuildLogicalTest(Token::AND, left, right);
+      VisitLogicalTest(Token::AND, left, right);
     }
     test_result->SetResultConsumedByTest();
   } else {
-    if (left->ToBooleanIsFalse()) {
-      VisitForAccumulatorValue(left);
-    } else if (left->ToBooleanIsTrue()) {
-      VisitForAccumulatorValue(right);
-    } else {
-      BytecodeLabel end_label;
-      TypeHint type_hint = VisitForAccumulatorValue(left);
-      builder()->JumpIfFalse(ToBooleanModeFromTypeHint(type_hint), &end_label);
-      VisitForAccumulatorValue(right);
-      builder()->Bind(&end_label);
-    }
+    BytecodeLabels end_labels(zone());
+    if (VisitLogicalAndSubExpression(left, &end_labels)) return;
+    VisitForAccumulatorValue(right);
+    end_labels.Bind(builder());
   }
 }
+
 void BytecodeGenerator::VisitNaryLogicalAndExpression(NaryOperation* expr) {
-  // TODO(leszeks): Implement.
-  UNREACHABLE();
+  Expression* first = expr->first();
+  DCHECK_GT(expr->subsequent_length(), 0);
+
+  if (execution_result()->IsTest()) {
+    TestResultScope* test_result = execution_result()->AsTest();
+    if (first->ToBooleanIsFalse()) {
+      builder()->Jump(test_result->NewElseLabel());
+    } else {
+      VisitNaryLogicalTest(Token::AND, expr);
+    }
+    test_result->SetResultConsumedByTest();
+  } else {
+    BytecodeLabels end_labels(zone());
+    if (VisitLogicalAndSubExpression(first, &end_labels)) return;
+    for (size_t i = 0; i < expr->subsequent_length() - 1; ++i) {
+      if (VisitLogicalAndSubExpression(expr->subsequent(i), &end_labels)) {
+        return;
+      }
+    }
+    // We have to visit the last value even if it's false, because we need its
+    // actual value.
+    VisitForAccumulatorValue(expr->subsequent(expr->subsequent_length() - 1));
+    end_labels.Bind(builder());
+  }
 }
 
 void BytecodeGenerator::VisitRewritableExpression(RewritableExpression* expr) {

src/interpreter/bytecode-generator.h

@@ -170,10 +170,22 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
   void VisitForInAssignment(Expression* expr);
   void VisitModuleNamespaceImports();
 
-  // Builds a logical OR/AND within a test context by rewiring the jumps based
+  // Visit a logical OR/AND within a test context, rewiring the jumps based
   // on the expression values.
-  void BuildLogicalTest(Token::Value token, Expression* left,
+  void VisitLogicalTest(Token::Value token, Expression* left,
                         Expression* right);
+  void VisitNaryLogicalTest(Token::Value token, NaryOperation* expr);
+  // Visit a (non-RHS) test for a logical op, which falls through if the test
+  // fails or jumps to the appropriate labels if it succeeds.
+  void VisitLogicalTestSubExpression(Token::Value token, Expression* expr,
+                                     BytecodeLabels* then_labels,
+                                     BytecodeLabels* else_labels);
+
+  // Helpers for binary and nary logical op value expressions.
+  bool VisitLogicalOrSubExpression(Expression* expr,
+                                   BytecodeLabels* end_labels);
+  bool VisitLogicalAndSubExpression(Expression* expr,
+                                    BytecodeLabels* end_labels);
 
   // Visit the header/body of a loop iteration.
   void VisitIterationHeader(IterationStatement* stmt,

src/parsing/parser.cc

@@ -310,10 +310,7 @@ bool Parser::ShortcutNumericLiteralBinaryExpression(Expression** x,
 bool Parser::CollapseNaryExpression(Expression** x, Expression* y,
                                     Token::Value op, int pos) {
   // Filter out unsupported ops.
-  // TODO(leszeks): Support AND and OR in bytecode generator.
-  if (!Token::IsBinaryOp(op) || op == Token::AND || op == Token::OR ||
-      op == Token::EXP)
-    return false;
+  if (!Token::IsBinaryOp(op) || op == Token::EXP) return false;
 
   // Convert *x into an nary operation with the given op, returning false if
   // this is not possible.

test/mjsunit/compiler/nary-binary-ops.js

@@ -3,7 +3,8 @@
 // found in the LICENSE file.
 
 // Test that n-ary chains of binary ops give an equal result to individual
-// binary op calls.
+// binary op calls. Also test binop chains inside an if condition return
+// the same branch.
 
 // Generate a function of the form
 //
@@ -49,6 +50,52 @@ function generate_nonchained_op(op, num_ops) {
     return eval(str);
 }
 
+// Generate a function of the form
+//
+// function(init,a0,...,aN) {
+//   if(init + a0 + ... + aN) return 1;
+//   else return 0;
+// }
+//
+// where + can be any binary operation.
+function generate_chained_op_test(op, num_ops) {
+    let str = "(function(init";
+    for (let i = 0; i < num_ops; i++) {
+        str += ",a"+i;
+    }
+    str += "){ if(init";
+    for (let i = 0; i < num_ops; i++) {
+        str += op+"a"+i;
+    }
+    str += ")return 1;else return 0;})";
+    return eval(str);
+}
+
+// Generate a function of the form
+//
+// function(init,a0,...,aN) {
+//   var tmp = init;
+//   tmp = tmp + a0;
+//   ...
+//   tmp = tmp + aN;
+//   if(tmp) return 1
+//  else return 0;
+// }
+//
+// where + can be any binary operation.
+function generate_nonchained_op_test(op, num_ops) {
+    let str = "(function(init";
+    for (let i = 0; i < num_ops; i++) {
+        str += ",a"+i;
+    }
+    str += "){ var tmp=init; ";
+    for (let i = 0; i < num_ops; i++) {
+        str += "tmp=(tmp"+op+"a"+i+");";
+    }
+    str += "if(tmp)return 1;else return 0;})";
+    return eval(str);
+}
+
 const BINOPS = [
     ",",
     "||",
@@ -69,8 +116,10 @@ const BINOPS = [
 // Test each binop to see if the chained version is equivalent to the non-
 // chained one.
 for (let op of BINOPS) {
-    let chained = generate_chained_op(op, 5);
-    let nonchained = generate_nonchained_op(op, 5);
+    let chained = generate_chained_op(op, 4);
+    let nonchained = generate_nonchained_op(op, 4);
+    let chained_test = generate_chained_op_test(op, 4);
+    let nonchained_test = generate_nonchained_op_test(op, 4);
 
     // With numbers.
     assertEquals(
@@ -83,4 +132,19 @@ for (let op of BINOPS) {
         nonchained(1,"2",3,"4",5),
         chained(1,"2",3,"4",5),
         "numeric and string " + op);
+
+    // Iterate over all possible combinations of 5 numbers that evaluate
+    // to boolean true or false (for testing logical ops).
+    for (var i = 0; i < 32; i++) {
+        var booleanArray = [i & 1, i & 2, i & 4, i & 8, i & 16];
+        assertEquals(
+            nonchained.apply(this, booleanArray),
+            chained.apply(this, booleanArray),
+            booleanArray.join(" " + op + " "));
+
+        assertEquals(
+            nonchained_test.apply(this, booleanArray),
+            chained_test.apply(this, booleanArray),
+            "if (" + booleanArray.join(" " + op + " ") + ")");
+    }
 }

test/mozilla/mozilla.status

@@ -152,6 +152,10 @@
 
   ##################### SLOW TESTS #####################
 
+  # Compiles a long chain of && or || operations, can time out under slower
+  # variants.
+  'js1_5/Expressions/regress-394673': [PASS, FAST_VARIANTS],
+
   # This takes a long time to run (~100 seconds). It should only be run
   # by the really patient.
   'js1_5/GC/regress-324278': [SKIP],
@@ -674,10 +678,6 @@
   # is given null or undefined as this argument (and so does firefox nightly).
   'js1_5/Regress/regress-295052': [FAIL],
 
-  # Bug 1202597: New js1_5/Expressions/regress-394673 is failing.
-  # Marked as: Will not fix. V8 throws an acceptable RangeError.
-  'js1_5/Expressions/regress-394673': [FAIL],
-
   # Bug 762: http://code.google.com/p/v8/issues/detail?id=762
   # We do not correctly handle assignments within "with"
   'ecma_3/Statements/12.10-01': [FAIL],