Get rid of CHECK_ALIVE, test only is_open during sequence node traversal.

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/flags.h"
 #include "vm/intermediate_language.h"
 #include "vm/longjump.h"
 #include "vm/os.h"
@@ skipping 116 matching lines @@
   true_successor_address_ = branch->true_successor_address();
   false_successor_address_ = branch->false_successor_address();
 }
 
 
 void EffectGraphVisitor::Bailout(const char* reason) {
   owner()->Bailout(reason);
 }
 
 
-// 'bailout' is a statement (without a semicolon), typically a return.
-#define CHECK_ALIVE(bailout)                            \
-  do {                                                  \
-    if (!is_open()) {                                   \
-      bailout;                                          \
-    }                                                   \
-  } while (false)
-
-
 // <Statement> ::= Return { value:                <Expression>
 //                          inlined_finally_list: <InlinedFinally>* }
 void EffectGraphVisitor::VisitReturnNode(ReturnNode* node) {
   ValueGraphVisitor for_value(owner(), temp_index());
   node->value()->Visit(&for_value);
   Append(for_value);
-  CHECK_ALIVE(return);
 
   for (intptr_t i = 0; i < node->inlined_finally_list_length(); i++) {
     EffectGraphVisitor for_effect(owner(), for_value.temp_index());
     node->InlinedFinallyNodeAt(i)->Visit(&for_effect);
     Append(for_effect);

[Kevin Millikin (Google), 2012/03/01 09:13:53]

I think we also need

if (!is_open()) return;

immediately after this call to append.  I can't construct an example because we
bail out on TryCatchNode.

I think the rule is: before connecting a successor to the graph fragment of an
EffectGraphVisitor, check and handle !is_open().

[srdjan, 2012/03/01 16:54:58]

Done.

-    CHECK_ALIVE(return);
   }
 
   Value* return_value = for_value.value();
   if (FLAG_enable_type_checks) {
     const RawFunction::Kind kind = owner()->parsed_function().function().kind();
     // Implicit getters do not need a type check at return.
     if ((kind != RawFunction::kImplicitGetter) &&
         (kind != RawFunction::kConstImplicitGetter)) {
       const AbstractType& type =
           AbstractType::ZoneHandle(
@@ skipping 35 matching lines @@
 
 
 // <Expression> :: Assignable { expr:     <Expression>
 //                              type:     AbstractType
 //                              dst_name: String }
 AssertAssignableComp* EffectGraphVisitor::TranslateAssignable(
     const AssignableNode& node) {
   ValueGraphVisitor for_value(owner(), temp_index());
   node.expr()->Visit(&for_value);
   Append(for_value);
-  CHECK_ALIVE(return NULL);
 
   return new AssertAssignableComp(for_value.value(), node.type());
 }
 
 void EffectGraphVisitor::VisitAssignableNode(AssignableNode* node) {
   AssertAssignableComp* assert = TranslateAssignable(*node);
-  CHECK_ALIVE(return);
   DoComputation(assert);
 }
 
 void ValueGraphVisitor::VisitAssignableNode(AssignableNode* node) {
   AssertAssignableComp* assert = TranslateAssignable(*node);
-  CHECK_ALIVE(return);
   ReturnValueOf(assert);
 }
 
 
 void TestGraphVisitor::VisitAssignableNode(AssignableNode* node) {
   AssertAssignableComp* assert = TranslateAssignable(*node);
-  CHECK_ALIVE(return);
   BranchOnValueOf(assert);
 }
 
 
 // <Expression> :: BinaryOp { kind:  Token::Kind
 //                            left:  <Expression>
 //                            right: <Expression> }
 InstanceCallComp* EffectGraphVisitor::TranslateBinaryOp(
     const BinaryOpNode& node) {
   // Operators "&&" and "||" cannot be overloaded therefore do not call
   // operator.
   if ((node.kind() == Token::kAND) || (node.kind() == Token::kOR)) {
     Bailout("EffectGraphVisitor::VisitBinaryOpNode AND/OR");
   }
   ValueGraphVisitor for_left_value(owner(), temp_index());
   node.left()->Visit(&for_left_value);
   Append(for_left_value);
-  CHECK_ALIVE(return NULL);
   ValueGraphVisitor for_right_value(owner(), for_left_value.temp_index());
   node.right()->Visit(&for_right_value);
   Append(for_right_value);
-  CHECK_ALIVE(return NULL);
   ZoneGrowableArray<Value*>* arguments = new ZoneGrowableArray<Value*>(2);
   arguments->Add(for_left_value.value());
   arguments->Add(for_right_value.value());
   return new InstanceCallComp(node.Name(), arguments);
 }
 
 void EffectGraphVisitor::VisitBinaryOpNode(BinaryOpNode* node) {
   InstanceCallComp* call = TranslateBinaryOp(*node);
-  CHECK_ALIVE(return);
   DoComputation(call);
 }
 
 void ValueGraphVisitor::VisitBinaryOpNode(BinaryOpNode* node) {
   InstanceCallComp* call = TranslateBinaryOp(*node);
-  CHECK_ALIVE(return);
   ReturnValueOf(call);
 }
 
 void TestGraphVisitor::VisitBinaryOpNode(BinaryOpNode* node) {
   InstanceCallComp* call = TranslateBinaryOp(*node);
-  CHECK_ALIVE(return);
   BranchOnValueOf(call);
 }
 
 
 void EffectGraphVisitor::VisitStringConcatNode(StringConcatNode* node) {
   Bailout("EffectGraphVisitor::VisitStringConcatNode");
 }
 void ValueGraphVisitor::VisitStringConcatNode(StringConcatNode* node) {
   Bailout("ValueGraphVisitor::VisitStringConcatNode");
 }
 void TestGraphVisitor::VisitStringConcatNode(StringConcatNode* node) {
   Bailout("TestGraphVisitor::VisitStringConcatNode");
 }
 
 
 // <Expression> :: Comparison { kind:  Token::Kind
 //                              left:  <Expression>
 //                              right: <Expression> }
 Computation* EffectGraphVisitor::TranslateComparison(
     const ComparisonNode& node) {
   if (Token::IsInstanceofOperator(node.kind())) {
     Bailout("instanceof not yet implemented");
   } else if ((node.kind() == Token::kEQ) || (node.kind() == Token::kNE)) {
     Bailout("'==' or '!=' comparison not yet implemented");
   }
   ValueGraphVisitor for_left_value(owner(), temp_index());
   node.left()->Visit(&for_left_value);
   Append(for_left_value);
-  CHECK_ALIVE(return NULL);
   ValueGraphVisitor for_right_value(owner(), for_left_value.temp_index());
   node.right()->Visit(&for_right_value);
   Append(for_right_value);
-  CHECK_ALIVE(return NULL);
   if ((node.kind() == Token::kEQ_STRICT) ||
       (node.kind() == Token::kNE_STRICT)) {
     return new StrictCompareComp(
         node.kind(), for_left_value.value(), for_right_value.value());
   }
   ZoneGrowableArray<Value*>* arguments = new ZoneGrowableArray<Value*>(2);
   arguments->Add(for_left_value.value());
   arguments->Add(for_right_value.value());
   return new InstanceCallComp(node.Name(), arguments);
 }
 
 void EffectGraphVisitor::VisitComparisonNode(ComparisonNode* node) {
   Computation* call = TranslateComparison(*node);
-  CHECK_ALIVE(return);
   DoComputation(call);
 }
 
 void ValueGraphVisitor::VisitComparisonNode(ComparisonNode* node) {
   Computation* call = TranslateComparison(*node);
-  CHECK_ALIVE(return);
   ReturnValueOf(call);
 }
 
 void TestGraphVisitor::VisitComparisonNode(ComparisonNode* node) {
   Computation* call = TranslateComparison(*node);
-  CHECK_ALIVE(return);
   BranchOnValueOf(call);
 }
 
 
 
 InstanceCallComp* EffectGraphVisitor::TranslateUnaryOp(
     const UnaryOpNode& node) {
   // "!" cannot be overloaded, therefore do not call operator.
   if (node.kind() == Token::kNOT) {
     Bailout("EffectGraphVisitor::VisitUnaryOpNode NOT");
@@ skipping 209 matching lines @@
 
 
 InstanceCallComp* EffectGraphVisitor::TranslateInstanceCall(
     const InstanceCallNode& node) {
   ArgumentListNode* arguments = node.arguments();
   int length = arguments->length();
   ZoneGrowableArray<Value*>* values = new ZoneGrowableArray<Value*>(length + 1);
   ValueGraphVisitor for_receiver(owner(), temp_index());
   node.receiver()->Visit(&for_receiver);
   Append(for_receiver);
-  CHECK_ALIVE(return NULL);
   values->Add(for_receiver.value());
   int index = temp_index();
   for (intptr_t i = 0; i < length; ++i) {
     ValueGraphVisitor for_value(owner(), index);
     arguments->NodeAt(i)->Visit(&for_value);
     Append(for_value);
-    CHECK_ALIVE(return NULL);
     values->Add(for_value.value());
     index = for_value.temp_index();
   }
   return new InstanceCallComp(node.function_name().ToCString(), values);
 }
 
 
 void EffectGraphVisitor::VisitInstanceCallNode(InstanceCallNode* node) {
   InstanceCallComp* call = TranslateInstanceCall(*node);
-  CHECK_ALIVE(return);
   DoComputation(call);
 }
 void ValueGraphVisitor::VisitInstanceCallNode(InstanceCallNode* node) {
   InstanceCallComp* call = TranslateInstanceCall(*node);
-  CHECK_ALIVE(return);
   ReturnValueOf(call);
 }
 void TestGraphVisitor::VisitInstanceCallNode(InstanceCallNode* node) {
   InstanceCallComp* call = TranslateInstanceCall(*node);
-  CHECK_ALIVE(return);
   BranchOnValueOf(call);
 }
 
 
 void EffectGraphVisitor::TranslateArgumentList(
     const ArgumentListNode& node, ZoneGrowableArray<Value*>* values) {
   int index = temp_index();
   for (intptr_t i = 0; i < node.length(); ++i) {
     ValueGraphVisitor for_value(owner(), index);
     node.NodeAt(i)->Visit(&for_value);
     Append(for_value);
-    CHECK_ALIVE(return);
     Value* argument_value = for_value.value();
     index = for_value.temp_index();
     if (argument_value->IsConstant()) {
       AddInstruction(new BindInstr(index, argument_value));
       argument_value = new TempVal(index++);
     }
     values->Add(argument_value);
   }
 }
 
 // <Expression> ::= StaticCall { function: Function
 //                               arguments: <ArgumentList> }
 StaticCallComp* EffectGraphVisitor::TranslateStaticCall(
     const StaticCallNode& node) {
   int length = node.arguments()->length();
   ZoneGrowableArray<Value*>* values = new ZoneGrowableArray<Value*>(length);
   TranslateArgumentList(*node.arguments(), values);
-  CHECK_ALIVE(return NULL);
   return new StaticCallComp(node.function(), values);
 }
 
 void EffectGraphVisitor::VisitStaticCallNode(StaticCallNode* node) {
   StaticCallComp* call = TranslateStaticCall(*node);
-  CHECK_ALIVE(return);
   DoComputation(call);
 }
 
 void ValueGraphVisitor::VisitStaticCallNode(StaticCallNode* node) {
   StaticCallComp* call = TranslateStaticCall(*node);
-  CHECK_ALIVE(return);
   ReturnValueOf(call);
 }
 
 void TestGraphVisitor::VisitStaticCallNode(StaticCallNode* node) {
   StaticCallComp* call = TranslateStaticCall(*node);
-  CHECK_ALIVE(return);
   BranchOnValueOf(call);
 }
 
 
 void EffectGraphVisitor::VisitClosureCallNode(ClosureCallNode* node) {
   Bailout("EffectGraphVisitor::VisitClosureCallNode");
 }
 void ValueGraphVisitor::VisitClosureCallNode(ClosureCallNode* node) {
   Bailout("ValueGraphVisitor::VisitClosureCallNode");
 }
@@ skipping 106 matching lines @@
 }
 
 
 // <Expression> ::= StoreLocal { local: LocalVariable
 //                               value: <Expression> }
 StoreLocalComp* EffectGraphVisitor::TranslateStoreLocal(
     const StoreLocalNode& node) {
   ValueGraphVisitor for_value(owner(), temp_index());
   node.value()->Visit(&for_value);
   Append(for_value);
-  CHECK_ALIVE(return NULL);
   return new StoreLocalComp(node.local(), for_value.value());
 }
 
 void EffectGraphVisitor::VisitStoreLocalNode(StoreLocalNode* node) {
   StoreLocalComp* store = TranslateStoreLocal(*node);
-  CHECK_ALIVE(return);
   DoComputation(store);
 }
 
 void ValueGraphVisitor::VisitStoreLocalNode(StoreLocalNode* node) {
   StoreLocalComp* store = TranslateStoreLocal(*node);
-  CHECK_ALIVE(return);
   ReturnValueOf(store);
 }
 
 void TestGraphVisitor::VisitStoreLocalNode(StoreLocalNode* node) {
   StoreLocalComp* store = TranslateStoreLocal(*node);
-  CHECK_ALIVE(return);
   BranchOnValueOf(store);
 }
 
 
 void EffectGraphVisitor::VisitLoadInstanceFieldNode(
     LoadInstanceFieldNode* node) {
   Bailout("EffectGraphVisitor::VisitLoadInstanceFieldNode");
 }
 void ValueGraphVisitor::VisitLoadInstanceFieldNode(
     LoadInstanceFieldNode* node) {
@@ skipping 63 matching lines @@
 
 
 // <Statement> ::= Sequence { scope: LocalScope
 //                            nodes: <Statement>*
 //                            label: SourceLabel }
 void EffectGraphVisitor::VisitSequenceNode(SequenceNode* node) {
   if ((node->scope() != NULL) &&
       (node->scope()->num_context_variables() != 0)) {
     Bailout("Sequence needs a context.  Gotta have a context.");
   }
-  for (intptr_t i = 0; i < node->length(); ++i) {
+  intptr_t i = 0;
+  while (is_open() && (i < node->length())) {
     EffectGraphVisitor for_effect(owner(), temp_index());
-    node->NodeAt(i)->Visit(&for_effect);
+    node->NodeAt(i++)->Visit(&for_effect);
     Append(for_effect);
-    CHECK_ALIVE(return);
   }
 }
 
 void ValueGraphVisitor::VisitSequenceNode(SequenceNode* node) { UNREACHABLE(); }
 void TestGraphVisitor::VisitSequenceNode(SequenceNode* node) { UNREACHABLE(); }
 
 
 void EffectGraphVisitor::VisitCatchClauseNode(CatchClauseNode* node) {
   Bailout("EffectGraphVisitor::VisitCatchClauseNode");
 }
@@ skipping 185 matching lines @@
   OS::Print("    if ");
   instr->value()->Accept(this);
   OS::Print(" goto(%d, %d)", instr->true_successor()->block_number(),
             instr->false_successor()->block_number());
 }
 
 
 void FlowGraphBuilder::BuildGraph() {
   EffectGraphVisitor for_effect(this, 0);
   for_effect.AddInstruction(new TargetEntryInstr());
   parsed_function().node_sequence()->Visit(&for_effect);

[Kevin Millikin (Google), 2012/03/01 09:13:53]

And after this call, I think we should be able to assert !for_effect.is_open(). 
(If that's not true today, we should make it true).

[srdjan, 2012/03/01 16:54:58]

Done.

   if (for_effect.entry() != NULL) {
     // Accumulate basic block entries via postorder traversal.
     for_effect.entry()->Postorder(&postorder_block_entries_);
     // Number the blocks in reverse postorder starting with 0.
     intptr_t last_index = postorder_block_entries_.length() - 1;
     for (intptr_t i = last_index; i >= 0; --i) {
       postorder_block_entries_[i]->set_block_number(last_index - i);
     }
   }
   if (FLAG_print_flow_graph) {
@@ skipping 10 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