Streamline code generator for branches and comparisons.

runtime/vm/intermediate_language_ia32.cc

Index: runtime/vm/intermediate_language_ia32.cc
===================================================================
--- runtime/vm/intermediate_language_ia32.cc	(revision 30042)
+++ runtime/vm/intermediate_language_ia32.cc	(working copy)
@@ -326,33 +326,38 @@
 }
 
 
+struct BranchLabels {
+  Label* true_label;
+  Label* false_label;
+  Label* fall_through;
+};
+
+
 static void EmitBranchOnValue(FlowGraphCompiler* compiler,
-                              TargetEntryInstr* true_successor,
-                              TargetEntryInstr* false_successor,
-                              bool value) {
-  if (value && !compiler->CanFallThroughTo(true_successor)) {
-    __ jmp(compiler->GetJumpLabel(true_successor));
-  } else if (!value && !compiler->CanFallThroughTo(false_successor)) {
-    __ jmp(compiler->GetJumpLabel(false_successor));
+                              bool value,
+                              BranchLabels labels) {
+  if (value && labels.fall_through != labels.true_label) {

[Kevin Millikin (Google), 2013/11/07 14:34:09]

I've actually gotten used to the extra parens around the != comparison.  Put
them here and below.

[Florian Schneider, 2013/11/08 12:00:20]

Done.

+    __ jmp(labels.true_label);
+  } else if (!value && labels.fall_through != labels.false_label) {
+    __ jmp(labels.false_label);
   }
 }
 
 
 static void EmitBranchOnCondition(FlowGraphCompiler* compiler,
-                                  TargetEntryInstr* true_successor,
-                                  TargetEntryInstr* false_successor,
-                                  Condition true_condition) {
-  if (compiler->CanFallThroughTo(false_successor)) {
+                                  Condition true_condition,
+                                  BranchLabels labels) {
+  if (labels.fall_through == labels.false_label) {
     // If the next block is the false successor, fall through to it.
-    __ j(true_condition, compiler->GetJumpLabel(true_successor));
+    __ j(true_condition, labels.true_label);
   } else {
     // If the next block is not the false successor, branch to it.
     Condition false_condition = NegateCondition(true_condition);
-    __ j(false_condition, compiler->GetJumpLabel(false_successor));
+    __ j(false_condition, labels.false_label);
 
     // Fall through or jump to the true successor.
-    if (!compiler->CanFallThroughTo(true_successor)) {
-      __ jmp(compiler->GetJumpLabel(true_successor));
+    if (labels.fall_through != labels.true_label) {
+      __ jmp(labels.true_label);
     }
   }
 }
@@ -361,7 +366,7 @@
 static void EmitSmiComparisonOp(FlowGraphCompiler* compiler,
                                 const LocationSummary& locs,
                                 Token::Kind kind,
-                                BranchInstr* branch) {
+                                BranchLabels labels) {
   Location left = locs.in(0);
   Location right = locs.in(1);
   ASSERT(!left.IsConstant() || !right.IsConstant());
@@ -378,22 +383,7 @@
   } else {
     __ cmpl(left.reg(), right.reg());
   }
-
-  if (branch != NULL) {
-    EmitBranchOnCondition(compiler,
-                          branch->true_successor(),
-                          branch->false_successor(),
-                          true_condition);
-  } else {
-    Register result = locs.out().reg();
-    Label done, is_true;
-    __ j(true_condition, &is_true);
-    __ LoadObject(result, Bool::False());
-    __ jmp(&done);
-    __ Bind(&is_true);
-    __ LoadObject(result, Bool::True());
-    __ Bind(&done);
-  }
+  EmitBranchOnCondition(compiler, true_condition, labels);
 }
 
 
@@ -441,7 +431,7 @@
 static void EmitUnboxedMintEqualityOp(FlowGraphCompiler* compiler,
                                       const LocationSummary& locs,
                                       Token::Kind kind,
-                                      BranchInstr* branch) {
+                                      BranchLabels labels) {
   ASSERT(Token::IsEqualityOperator(kind));
   XmmRegister left = locs.in(0).fpu_reg();
   XmmRegister right = locs.in(1).fpu_reg();
@@ -452,34 +442,18 @@
 
   Condition true_condition = TokenKindToMintCondition(kind);
   __ cmpl(temp, Immediate(-1));
-
-  if (branch != NULL) {
-    EmitBranchOnCondition(compiler,
-                          branch->true_successor(),
-                          branch->false_successor(),
-                          true_condition);
-  } else {
-    Register result = locs.out().reg();
-    Label done, is_true;
-    __ j(true_condition, &is_true);
-    __ LoadObject(result, Bool::False());
-    __ jmp(&done);
-    __ Bind(&is_true);
-    __ LoadObject(result, Bool::True());
-    __ Bind(&done);
-  }
+  EmitBranchOnCondition(compiler, true_condition, labels);
 }
 
 
 static void EmitUnboxedMintComparisonOp(FlowGraphCompiler* compiler,
                                         const LocationSummary& locs,
                                         Token::Kind kind,
-                                        BranchInstr* branch) {
+                                        BranchLabels labels) {
   XmmRegister left = locs.in(0).fpu_reg();
   XmmRegister right = locs.in(1).fpu_reg();
   Register left_tmp = locs.temp(0).reg();
   Register right_tmp = locs.temp(1).reg();
-  Register result = branch == NULL ? locs.out().reg() : kNoRegister;
 
   Condition hi_cond = OVERFLOW, lo_cond = OVERFLOW;
   switch (kind) {
@@ -508,34 +482,14 @@
   __ pextrd(left_tmp, left, Immediate(1));
   __ pextrd(right_tmp, right, Immediate(1));
   __ cmpl(left_tmp, right_tmp);
-  if (branch != NULL) {
-    __ j(hi_cond, compiler->GetJumpLabel(branch->true_successor()));
-    __ j(FlipCondition(hi_cond),
-         compiler->GetJumpLabel(branch->false_successor()));
-  } else {
-    __ j(hi_cond, &is_true);
-    __ j(FlipCondition(hi_cond), &is_false);
-  }
+  __ j(hi_cond, labels.true_label);
+  __ j(FlipCondition(hi_cond), labels.false_label);
 
   // If upper is equal, compare lower half.
   __ pextrd(left_tmp, left, Immediate(0));
   __ pextrd(right_tmp, right, Immediate(0));
   __ cmpl(left_tmp, right_tmp);
-  if (branch != NULL) {
-    EmitBranchOnCondition(compiler,
-                          branch->true_successor(),
-                          branch->false_successor(),
-                          lo_cond);
-  } else {
-    Label done;
-    __ j(lo_cond, &is_true);
-    __ Bind(&is_false);
-    __ LoadObject(result, Bool::False());
-    __ jmp(&done);
-    __ Bind(&is_true);
-    __ LoadObject(result, Bool::True());
-    __ Bind(&done);
-  }
+  EmitBranchOnCondition(compiler, lo_cond, labels);
 }
 
 
@@ -554,97 +508,74 @@
 }
 
 
-static void EmitDoubleCompareBranch(FlowGraphCompiler* compiler,
-                                    Condition true_condition,
-                                    FpuRegister left,
-                                    FpuRegister right,
-                                    BranchInstr* branch) {
-  ASSERT(branch != NULL);
-  __ comisd(left, right);
-  BlockEntryInstr* nan_result = (true_condition == NOT_EQUAL) ?
-      branch->true_successor() : branch->false_successor();
-  __ j(PARITY_EVEN, compiler->GetJumpLabel(nan_result));
-  EmitBranchOnCondition(compiler,
-                        branch->true_successor(),
-                        branch->false_successor(),
-                        true_condition);
-}
-
-
-
-static void EmitDoubleCompareBool(FlowGraphCompiler* compiler,
-                                  Condition true_condition,
-                                  FpuRegister left,
-                                  FpuRegister right,
-                                  Register result) {
-  __ comisd(left, right);
-  Label is_false, is_true, done;
-  // x == NaN -> false, x != NaN -> true.
-  Label* nan_label = (true_condition == NOT_EQUAL) ? &is_true : &is_false;
-  __ j(PARITY_EVEN, nan_label, Assembler::kNearJump);
-  __ j(true_condition, &is_true, Assembler::kNearJump);
-  __ Bind(&is_false);
-  __ LoadObject(result, Bool::False());
-  __ jmp(&done);
-  __ Bind(&is_true);
-  __ LoadObject(result, Bool::True());
-  __ Bind(&done);
-}
-
-
 static void EmitDoubleComparisonOp(FlowGraphCompiler* compiler,
                                    const LocationSummary& locs,
                                    Token::Kind kind,
-                                   BranchInstr* branch) {
+                                   BranchLabels labels) {
   XmmRegister left = locs.in(0).fpu_reg();
   XmmRegister right = locs.in(1).fpu_reg();
 
+  __ comisd(left, right);
+
   Condition true_condition = TokenKindToDoubleCondition(kind);
-  if (branch != NULL) {
-    EmitDoubleCompareBranch(compiler, true_condition, left, right, branch);
-  } else {
-    EmitDoubleCompareBool(compiler, true_condition,
-                          left, right, locs.out().reg());
-  }
+  Label* nan_result = (true_condition == NOT_EQUAL)
+      ? labels.true_label : labels.false_label;
+  __ j(PARITY_EVEN, nan_result);
+  EmitBranchOnCondition(compiler, true_condition, labels);
 }
 
 
+static BranchLabels CreateBranchLabels(FlowGraphCompiler* compiler,
+                                       BranchInstr* branch) {
+  Label* true_label = compiler->GetJumpLabel(branch->true_successor());
+  Label* false_label = compiler->GetJumpLabel(branch->false_successor());
+  Label* fall_through = compiler->CanFallThroughTo(branch->true_successor())

[Kevin Millikin (Google), 2013/11/07 14:34:09]

Hmmm, that's kind of ugly.  What about a function to get the label of the next
non-empty block:

Label* FlowGraphCompiler::NextNonemptyLabel(BlockEntryInstr* block_entry) const
{
  const intptr_t current_index = current_block()->postorder_number();
  return block_info_[current_index]->next_nonempty_label();
}

And set fall_through to that --- it doesn't have to be NULL, just some pointer
distinct from true_label and false_label.  Or do you think that's too
error-prone?

[Florian Schneider, 2013/11/08 12:00:20]

Agreed. CanFallThroughTo uses next_nonempty_label anyway, so it is as good.

+      ? true_label : compiler->CanFallThroughTo(branch->false_successor())
+          ? false_label : NULL;
+  BranchLabels result = { true_label, false_label, fall_through };
+  return result;
+}
+
+
 void EqualityCompareInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   ASSERT((kind() == Token::kNE) || (kind() == Token::kEQ));
-  BranchInstr* kNoBranch = NULL;
+
+  Label is_true, is_false;
+  BranchLabels labels = { &is_true, &is_false, &is_false };
+
   if (operation_cid() == kSmiCid) {
-    EmitSmiComparisonOp(compiler, *locs(), kind(), kNoBranch);
-    return;
+    EmitSmiComparisonOp(compiler, *locs(), kind(), labels);
+  } else if (operation_cid() == kMintCid) {
+    EmitUnboxedMintEqualityOp(compiler, *locs(), kind(), labels);
+  } else {
+    ASSERT(operation_cid() == kDoubleCid);
+    EmitDoubleComparisonOp(compiler, *locs(), kind(), labels);
   }
-  if (operation_cid() == kMintCid) {
-    EmitUnboxedMintEqualityOp(compiler, *locs(), kind(), kNoBranch);
-    return;
-  }
-  if (operation_cid() == kDoubleCid) {
-    EmitDoubleComparisonOp(compiler, *locs(), kind(), kNoBranch);
-    return;
-  }
-  UNREACHABLE();
+  Register result = locs()->out().reg();
+  Label done;
+  __ Bind(&is_false);
+  __ LoadObject(result, Bool::False());
+  __ jmp(&done, Assembler::kNearJump);
+  __ Bind(&is_true);
+  __ LoadObject(result, Bool::True());
+  __ Bind(&done);
 }
 
 
 void EqualityCompareInstr::EmitBranchCode(FlowGraphCompiler* compiler,
                                           BranchInstr* branch) {
   ASSERT((kind() == Token::kNE) || (kind() == Token::kEQ));
+
+  BranchLabels labels = CreateBranchLabels(compiler, branch);
+
   if (operation_cid() == kSmiCid) {
-    // Deoptimizes if both arguments not Smi.
-    EmitSmiComparisonOp(compiler, *locs(), kind(), branch);
-    return;
+    EmitSmiComparisonOp(compiler, *locs(), kind(), labels);
+  } else if (operation_cid() == kMintCid) {
+    EmitUnboxedMintEqualityOp(compiler, *locs(), kind(), labels);
+  } else {
+    ASSERT(operation_cid() == kDoubleCid);
+    EmitDoubleComparisonOp(compiler, *locs(), kind(), labels);
   }
-  if (operation_cid() == kMintCid) {
-    EmitUnboxedMintEqualityOp(compiler, *locs(), kind(), branch);
-    return;
-  }
-  if (operation_cid() == kDoubleCid) {
-    EmitDoubleComparisonOp(compiler, *locs(), kind(), branch);
-    return;
-  }
-  UNREACHABLE();
 }
 
 
@@ -669,7 +600,9 @@
 
 void TestSmiInstr::EmitBranchCode(FlowGraphCompiler* compiler,
                                   BranchInstr* branch) {
-  Condition branch_condition = (kind() == Token::kNE) ? NOT_ZERO : ZERO;
+  BranchLabels labels = CreateBranchLabels(compiler, branch);
+
+  Condition true_condition = (kind() == Token::kNE) ? NOT_ZERO : ZERO;
   Register left = locs()->in(0).reg();
   Location right = locs()->in(1);
   if (right.IsConstant()) {
@@ -680,10 +613,7 @@
   } else {
     __ testl(left, right.reg());
   }
-  EmitBranchOnCondition(compiler,
-                        branch->true_successor(),
-                        branch->false_successor(),
-                        branch_condition);
+  EmitBranchOnCondition(compiler, true_condition, labels);
 }
 
 
@@ -724,31 +654,40 @@
 
 
 void RelationalOpInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
+  Label is_true, is_false;
+  BranchLabels labels = { &is_true, &is_false, &is_false };
+
   if (operation_cid() == kSmiCid) {
-    EmitSmiComparisonOp(compiler, *locs(), kind(), NULL);
-    return;
+    EmitSmiComparisonOp(compiler, *locs(), kind(), labels);
+  } else if (operation_cid() == kMintCid) {
+    EmitUnboxedMintComparisonOp(compiler, *locs(), kind(), labels);
+  } else {
+    ASSERT(operation_cid() == kDoubleCid);
+    EmitDoubleComparisonOp(compiler, *locs(), kind(), labels);
   }
-  if (operation_cid() == kMintCid) {
-    EmitUnboxedMintComparisonOp(compiler, *locs(), kind(), NULL);
-    return;
-  }
-  ASSERT(operation_cid() == kDoubleCid);
-  EmitDoubleComparisonOp(compiler, *locs(), kind(), NULL);
+  Register result = locs()->out().reg();
+  Label done;
+  __ Bind(&is_false);
+  __ LoadObject(result, Bool::False());
+  __ jmp(&done, Assembler::kNearJump);
+  __ Bind(&is_true);
+  __ LoadObject(result, Bool::True());
+  __ Bind(&done);
 }
 
 
 void RelationalOpInstr::EmitBranchCode(FlowGraphCompiler* compiler,
                                        BranchInstr* branch) {
+  BranchLabels labels = CreateBranchLabels(compiler, branch);
+
   if (operation_cid() == kSmiCid) {
-    EmitSmiComparisonOp(compiler, *locs(), kind(), branch);
-    return;
+    EmitSmiComparisonOp(compiler, *locs(), kind(), labels);
+  } else if (operation_cid() == kMintCid) {
+    EmitUnboxedMintComparisonOp(compiler, *locs(), kind(), labels);
+  } else {
+    ASSERT(operation_cid() == kDoubleCid);
+    EmitDoubleComparisonOp(compiler, *locs(), kind(), labels);
   }
-  if (operation_cid() == kMintCid) {
-    EmitUnboxedMintComparisonOp(compiler, *locs(), kind(), branch);
-    return;
-  }
-  ASSERT(operation_cid() == kDoubleCid);
-  EmitDoubleComparisonOp(compiler, *locs(), kind(), branch);
 }
 
 
@@ -4656,43 +4595,59 @@
 }
 
 
-// Special code for numbers (compare values instead of references.)
-void StrictCompareInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
-  ASSERT(kind() == Token::kEQ_STRICT || kind() == Token::kNE_STRICT);
-  Location left = locs()->in(0);
-  Location right = locs()->in(1);
+static void EmitStrictComparison(FlowGraphCompiler* compiler,
+                                 StrictCompareInstr* compare,
+                                 BranchLabels labels) {
+  LocationSummary* locs = compare->locs();
+  bool needs_number_check = compare->needs_number_check();
+  intptr_t token_pos = compare->token_pos();
+  Token::Kind kind = compare->kind();
+  Location left = locs->in(0);
+  Location right = locs->in(1);
   if (left.IsConstant() && right.IsConstant()) {
     // TODO(vegorov): should be eliminated earlier by constant propagation.
-    const bool result = (kind() == Token::kEQ_STRICT) ?
+    const bool result = (kind == Token::kEQ_STRICT) ?
         left.constant().raw() == right.constant().raw() :
         left.constant().raw() != right.constant().raw();
-    __ LoadObject(locs()->out().reg(), Bool::Get(result));
+    EmitBranchOnValue(compiler, result, labels);
     return;
   }
   if (left.IsConstant()) {
     compiler->EmitEqualityRegConstCompare(right.reg(),
                                           left.constant(),
-                                          needs_number_check(),
-                                          token_pos());
+                                          needs_number_check,
+                                          token_pos);
   } else if (right.IsConstant()) {
     compiler->EmitEqualityRegConstCompare(left.reg(),
                                           right.constant(),
-                                          needs_number_check(),
-                                          token_pos());
+                                          needs_number_check,
+                                          token_pos);
   } else {
     compiler->EmitEqualityRegRegCompare(left.reg(),
-                                       right.reg(),
-                                       needs_number_check(),
-                                       token_pos());
+                                        right.reg(),
+                                        needs_number_check,
+                                        token_pos);
   }
+  Condition true_condition = (kind == Token::kEQ_STRICT) ? EQUAL : NOT_EQUAL;
+  EmitBranchOnCondition(compiler, true_condition, labels);
+}
 
+
+// Special code for numbers (compare values instead of references.)
+void StrictCompareInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
+  ASSERT(kind() == Token::kEQ_STRICT || kind() == Token::kNE_STRICT);
+
+  Label is_true, is_false;
+  BranchLabels labels = { &is_true, &is_false, &is_false };
+
+  EmitStrictComparison(compiler, this, labels);
+
   Register result = locs()->out().reg();
-  Label load_true, done;
-  Condition true_condition = (kind() == Token::kEQ_STRICT) ? EQUAL : NOT_EQUAL;
-  __ j(true_condition, &load_true, Assembler::kNearJump);
+  Label done;
+  __ Bind(&is_false);
   __ LoadObject(result, Bool::False());
   __ jmp(&done, Assembler::kNearJump);
-  __ Bind(&load_true);
+  __ Bind(&is_true);
   __ LoadObject(result, Bool::True());
   __ Bind(&done);
 }
@@ -4701,41 +4656,10 @@
 void StrictCompareInstr::EmitBranchCode(FlowGraphCompiler* compiler,
                                         BranchInstr* branch) {
   ASSERT(kind() == Token::kEQ_STRICT || kind() == Token::kNE_STRICT);
-  Location left = locs()->in(0);
-  Location right = locs()->in(1);
-  if (left.IsConstant() && right.IsConstant()) {
-    // TODO(vegorov): should be eliminated earlier by constant propagation.
-    const bool result = (kind() == Token::kEQ_STRICT) ?
-        left.constant().raw() == right.constant().raw() :
-        left.constant().raw() != right.constant().raw();
-    EmitBranchOnValue(compiler,
-                      branch->true_successor(),
-                      branch->false_successor(),
-                      result);
-    return;
-  }
-  if (left.IsConstant()) {
-    compiler->EmitEqualityRegConstCompare(right.reg(),
-                                          left.constant(),
-                                          needs_number_check(),
-                                          token_pos());
-  } else if (right.IsConstant()) {
-    compiler->EmitEqualityRegConstCompare(left.reg(),
-                                          right.constant(),
-                                          needs_number_check(),
-                                          token_pos());
-  } else {
-    compiler->EmitEqualityRegRegCompare(left.reg(),
-                                        right.reg(),
-                                        needs_number_check(),
-                                        token_pos());
-  }
 
-  Condition true_condition = (kind() == Token::kEQ_STRICT) ? EQUAL : NOT_EQUAL;
-  EmitBranchOnCondition(compiler,
-                        branch->true_successor(),
-                        branch->false_successor(),
-                        true_condition);
+  BranchLabels labels = CreateBranchLabels(compiler, branch);
+
+  EmitStrictComparison(compiler, this, labels);
 }