Implement logical AND/OR.

runtime/vm/flow_graph_builder.cc

Index: runtime/vm/flow_graph_builder.cc
===================================================================
--- runtime/vm/flow_graph_builder.cc	(revision 5196)
+++ runtime/vm/flow_graph_builder.cc	(working copy)
@@ -219,9 +219,21 @@
   // Operators "&&" and "||" cannot be overloaded therefore do not call
   // operator.
   if ((node->kind() == Token::kAND) || (node->kind() == Token::kOR)) {
-    // Implement short-circuit logic: do not evaluate right if evaluation
-    // of left is sufficient.
-    Bailout("EffectGraphVisitor::VisitBinaryOpNode AND/OR");
+    // See ValueGraphVisitor::VisitBinaryOpNode.
+    TestGraphVisitor for_test(owner(), temp_index());
+    node->left()->Visit(&for_test);
+    if (node->kind() == Token::kAND) {

[Kevin Millikin (Google), 2012/03/09 12:29:14]

Maybe too cute is to write:

EffectGraphVisitor for_right(owner(), temp_index());
node->right()->Visit(&for_right);
EffectGraphVisitor empty(owner(), temp_index());
if (node->kind() == Token::kAND) {
  Join(for_left, for_right, empty);
} else {
  Join(for_left, empty, for_right);
}

[srdjan, 2012/03/09 19:47:41]

Done.

+      EffectGraphVisitor for_true(owner(), temp_index());
+      node->right()->Visit(&for_true);
+      EffectGraphVisitor for_false(owner(), temp_index());
+      Join(for_test, for_true, for_false);
+    } else {
+      EffectGraphVisitor for_true(owner(), temp_index());
+      EffectGraphVisitor for_false(owner(), temp_index());
+      node->right()->Visit(&for_false);
+      Join(for_test, for_true, for_false);
+    }
+    return;
   }
   ArgumentGraphVisitor for_left_value(owner(), temp_index());
   node->left()->Visit(&for_left_value);
@@ -240,6 +252,64 @@
 }
 
 
+// Special handling for AND/OR.
+void ValueGraphVisitor::VisitBinaryOpNode(BinaryOpNode* node) {
+  // Operators "&&" and "||" cannot be overloaded therefore do not call
+  // operator.
+  if ((node->kind() == Token::kAND) || (node->kind() == Token::kOR)) {
+    // Implement short-circuit logic: do not evaluate right if evaluation
+    // of left is sufficient.
+    // AND:  left ? right : false;

[Kevin Millikin (Google), 2012/03/09 12:29:14]

right needs a conversion to boolean (e.g., right === true).  This translation
won't work for:

print(true && 0);

Then, in a test context, we can do better than testing the literal false when
left is not true---translate right in a test context as well.  (Not as an
optimization in the non-optimizing compiler, but because that's what we want in
the optimizing compiler and it's easier if we keep the graph with the same shape
early.)

[srdjan, 2012/03/09 19:47:41]

Thanks for catching it. Added tests (separate CL). There are two possibilities:
one is to use StrictCompare to convert right to boolean, the other is to use the
test context. I assume you would prefer the second, and I agree that it would
better convey the intent. However, I gave up on it (could not understand the
visitor interaction after many attempts) and implemented the correct version
using StrictCompare test.

+    // OR:   left ? true : right;
+    if (FLAG_enable_type_checks) {
+      Bailout("GenerateConditionTypeCheck in kAND/kOR");
+    }
+    const Bool& bool_true = Bool::ZoneHandle(Bool::True());
+    const Bool& bool_false = Bool::ZoneHandle(Bool::False());
+
+    TestGraphVisitor for_test(owner(), temp_index());
+    node->left()->Visit(&for_test);
+
+    if (node->kind() == Token::kAND) {
+      ValueGraphVisitor for_true(owner(), temp_index());
+      node->right()->Visit(&for_true);
+      if (for_true.value()->IsTemp()) {
+        ASSERT(for_true.value()->AsTemp()->index() == temp_index());
+      } else {
+        for_true.AddInstruction(
+            new BindInstr(temp_index(), for_true.value()));
+      }
+
+      ValueGraphVisitor for_false(owner(), temp_index());
+      for_false.AddInstruction(
+          new BindInstr(temp_index(), new ConstantVal(bool_false)));
+
+      Join(for_test, for_true, for_false);
+      ReturnValue(new TempVal(AllocateTempIndex()));
+    } else {
+      ASSERT(node->kind() == Token::kOR);
+      ValueGraphVisitor for_true(owner(), temp_index());
+      for_true.AddInstruction(
+          new BindInstr(temp_index(), new ConstantVal(bool_true)));
+
+      ValueGraphVisitor for_false(owner(), temp_index());
+      node->right()->Visit(&for_false);
+      if (for_false.value()->IsTemp()) {
+        ASSERT(for_false.value()->AsTemp()->index() == temp_index());
+      } else {
+        for_false.AddInstruction(
+            new BindInstr(temp_index(), for_false.value()));
+      }
+
+      Join(for_test, for_true, for_false);
+      ReturnValue(new TempVal(AllocateTempIndex()));
+    }
+    return;
+  }
+  EffectGraphVisitor::VisitBinaryOpNode(node);
+}
+
+
 void EffectGraphVisitor::VisitStringConcatNode(StringConcatNode* node) {
   Bailout("EffectGraphVisitor::VisitStringConcatNode");
 }