Implement logical AND/OR.

runtime/vm/flow_graph_builder.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/flow_graph_builder.h"
 
 #include "vm/ast_printer.h"
 #include "vm/flags.h"
 #include "vm/intermediate_language.h"
 #include "vm/longjump.h"
@@ skipping 201 matching lines @@
 }
 
 
 // <Expression> :: BinaryOp { kind:  Token::Kind
 //                            left:  <Expression>
 //                            right: <Expression> }
 void EffectGraphVisitor::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.
-    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);
   Append(for_left_value);
   ArgumentGraphVisitor for_right_value(owner(), for_left_value.temp_index());
   node->right()->Visit(&for_right_value);
   Append(for_right_value);
   ZoneGrowableArray<Value*>* arguments = new ZoneGrowableArray<Value*>(2);
   arguments->Add(for_left_value.value());
   arguments->Add(for_right_value.value());
   const String& name = String::ZoneHandle(String::NewSymbol(node->Name()));
   InstanceCallComp* call =
       new InstanceCallComp(node->id(), node->token_index(), name,
                            arguments, Array::ZoneHandle(), 2);
   ReturnComputation(call);
 }
 
 
+// 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");
 }
 
 
 // <Expression> :: Comparison { kind:  Token::Kind
 //                              left:  <Expression>
 //                              right: <Expression> }
 void EffectGraphVisitor::VisitComparisonNode(ComparisonNode* node) {
   if (Token::IsInstanceofOperator(node->kind())) {
@@ skipping 923 matching lines @@
   char* chars = reinterpret_cast<char*>(
       Isolate::Current()->current_zone()->Allocate(len));
   OS::SNPrint(chars, len, kFormat, function_name, reason);
   const Error& error = Error::Handle(
       LanguageError::New(String::Handle(String::New(chars))));
   Isolate::Current()->long_jump_base()->Jump(1, error);
 }
 
 
 }  // namespace dart