DBC: Various instructions.

runtime/vm/intermediate_language_dbc.cc

Index: runtime/vm/intermediate_language_dbc.cc
diff --git a/runtime/vm/intermediate_language_dbc.cc b/runtime/vm/intermediate_language_dbc.cc
index cfbfaa8ca07efc16546b90d81baea243f638e62e..99a31a7d33cf276b8213f806cab14d3a17f086d5 100644
--- a/runtime/vm/intermediate_language_dbc.cc
+++ b/runtime/vm/intermediate_language_dbc.cc
@@ -101,10 +101,6 @@ DECLARE_FLAG(int, optimization_counter_threshold);
   M(BoxInteger32)                                                              \
   M(UnboxInteger32)                                                            \
   M(CheckedSmiOp)                                                              \
-  M(CheckArrayBound)                                                           \
-  M(RelationalOp)                                                              \
-  M(EqualityCompare)                                                           \
-  M(LoadIndexed)
 
 // Location summaries actually are not used by the unoptimizing DBC compiler
 // because we don't allocate any registers.
@@ -169,8 +165,6 @@ FOR_EACH_UNIMPLEMENTED_INSTRUCTION(DEFINE_UNIMPLEMENTED)
 #undef DEFINE_UNIMPLEMENTED
 
 DEFINE_UNIMPLEMENTED_EMIT_BRANCH_CODE(TestCids)
-DEFINE_UNIMPLEMENTED_EMIT_BRANCH_CODE(RelationalOp)
-DEFINE_UNIMPLEMENTED_EMIT_BRANCH_CODE(EqualityCompare)
 
 
 EMIT_NATIVE_CODE(InstanceOf, 2, Location::SameAsFirstInput(),
@@ -582,6 +576,23 @@ EMIT_NATIVE_CODE(StoreIndexed, 3) {
 }
 
 
+EMIT_NATIVE_CODE(LoadIndexed, 2, Location::RequiresRegister()) {
+  ASSERT(compiler->is_optimizing());
+  if (class_id() != kArrayCid) {
+#if defined(PRODUCT)
+    compiler->Bailout("LoadIndexed");
+#else  // defined(PRODUCT)
+    compiler->Bailout(ToCString());
+#endif  // defined(PRODUCT)
+  }
+  const Register array = locs()->in(0).reg();
+  const Register index = locs()->in(1).reg();
+  const Register result = locs()->out(0).reg();
+
+  __ LoadIndexed(result, array, index);
+}
+
+
 EMIT_NATIVE_CODE(StringInterpolate,
                  1, Location::RegisterLocation(0),
                  LocationSummary::kCall) {
@@ -649,7 +660,6 @@ EMIT_NATIVE_CODE(StringToCharCode,
 }
 
 
-
 EMIT_NATIVE_CODE(AllocateObject,
                  0, Location::RequiresRegister(),
                  LocationSummary::kCall) {
@@ -748,10 +758,10 @@ EMIT_NATIVE_CODE(CatchBlockEntry, 0) {
 
 EMIT_NATIVE_CODE(Throw, 0, Location::NoLocation(), LocationSummary::kCall) {
   __ Throw(0);
-  compiler->RecordSafepoint(locs());
   compiler->AddCurrentDescriptor(RawPcDescriptors::kOther,
                                  deopt_id(),
                                  token_pos());
+  compiler->RecordAfterCall(this);
   __ Trap();
 }
 
@@ -759,10 +769,10 @@ EMIT_NATIVE_CODE(Throw, 0, Location::NoLocation(), LocationSummary::kCall) {
 EMIT_NATIVE_CODE(ReThrow, 0, Location::NoLocation(), LocationSummary::kCall) {
   compiler->SetNeedsStacktrace(catch_try_index());
   __ Throw(1);
-  compiler->RecordSafepoint(locs());
   compiler->AddCurrentDescriptor(RawPcDescriptors::kOther,
                                  deopt_id(),
                                  token_pos());
+  compiler->RecordAfterCall(this);
   __ Trap();
 }
 
@@ -1163,6 +1173,139 @@ EMIT_NATIVE_CODE(UnarySmiOp, 1, Location::RequiresRegister()) {
   }
 }
 
+
+static Token::Kind FlipCondition(Token::Kind kind) {
+  switch (kind) {
+    case Token::kEQ: return Token::kNE;
+    case Token::kNE: return Token::kEQ;
+    case Token::kLT: return Token::kGTE;
+    case Token::kGT: return Token::kLTE;
+    case Token::kLTE: return Token::kGT;
+    case Token::kGTE: return Token::kLT;
+    default:
+      UNREACHABLE();
+      return Token::kNE;
+  }
+}
+
+
+static Bytecode::Opcode OpcodeForCondition(Token::Kind kind) {
+  switch (kind) {
+    case Token::kEQ: return Bytecode::kIfEqStrict;
+    case Token::kNE: return Bytecode::kIfNeStrict;
+    case Token::kLT: return Bytecode::kIfLt;
+    case Token::kGT: return Bytecode::kIfGt;
+    case Token::kLTE: return Bytecode::kIfLe;
+    case Token::kGTE: return Bytecode::kIfGe;
+    default:
+      UNREACHABLE();
+      return Bytecode::kTrap;
+  }
+}
+
+
+static Condition EmitSmiComparisonOp(FlowGraphCompiler* compiler,
+                                     LocationSummary* locs,
+                                     Token::Kind kind,
+                                     BranchLabels labels) {
+  const Register left = locs->in(0).reg();
+  const Register right = locs->in(1).reg();
+  Token::Kind comparison = kind;
+  Condition condition = NEXT_IS_TRUE;
+  if (labels.fall_through != labels.false_label) {

[Vyacheslav Egorov (Google), 2016/07/01 16:15:35]

I don't get how this works. If label.fall_through is equal to labels.false_label
that means NEXT_IS_FALSE.

[zra, 2016/07/01 17:43:44]

Added some comments here and to EmitBranchOnCondition.

+    // Flip comparision to save a jump.
+    condition = NEXT_IS_FALSE;
+    comparison = FlipCondition(kind);
+  }
+  __ Emit(Bytecode::Encode(OpcodeForCondition(comparison), left, right));
+  return condition;
+}
+
+
+Condition EqualityCompareInstr::EmitComparisonCode(FlowGraphCompiler* compiler,
+                                                   BranchLabels labels) {
+  if (operation_cid() == kSmiCid) {
+    return EmitSmiComparisonOp(compiler, locs(), kind(), labels);
+  } else {
+    ASSERT(operation_cid() == kDoubleCid);
+#if defined(PRODUCT)
+    compiler->Bailout("EqualityCompareInstr::EmitComparisonCode");
+#else  // defined(PRODUCT)
+    compiler->Bailout(ToCString());
+#endif  // defined(PRODUCT)
+    return NEXT_IS_FALSE;
+  }
+}
+
+
+EMIT_NATIVE_CODE(EqualityCompare, 2, Location::RequiresRegister()) {
+  ASSERT(compiler->is_optimizing());
+  ASSERT((kind() == Token::kEQ) || (kind() == Token::kNE));
+  Label is_true, is_false;
+  BranchLabels labels = { &is_true, &is_false, &is_false };

[Vyacheslav Egorov (Google), 2016/07/01 16:15:35]

This does not match the code below. This configuration of labels means that
falltrough is false case.

[zra, 2016/07/01 17:43:45]

The labels don't make sense here because we aren't jumping. They're just picked
so that EmitComparisonCode will return NEXT_IS_TRUE. I'll add a comment.

+  const Register result = locs()->out(0).reg();
+  __ LoadConstant(result, Bool::False());
+  Condition true_condition = EmitComparisonCode(compiler, labels);
+  ASSERT(true_condition == NEXT_IS_TRUE);
+  __ LoadConstant(result, Bool::True());
+}
+
+
+void EqualityCompareInstr::EmitBranchCode(FlowGraphCompiler* compiler,
+                                          BranchInstr* branch) {
+  ASSERT((kind() == Token::kNE) || (kind() == Token::kEQ));
+  BranchLabels labels = compiler->CreateBranchLabels(branch);
+  Condition true_condition = EmitComparisonCode(compiler, labels);
+  EmitBranchOnCondition(compiler, true_condition, labels);
+}
+
+
+Condition RelationalOpInstr::EmitComparisonCode(FlowGraphCompiler* compiler,
+                                                BranchLabels labels) {
+  if (operation_cid() == kSmiCid) {
+    return EmitSmiComparisonOp(compiler, locs(), kind(), labels);
+  } else {
+    ASSERT(operation_cid() == kDoubleCid);
+#if defined(PRODUCT)
+    compiler->Bailout("EqualityCompareInstr::EmitComparisonCode");
+#else  // defined(PRODUCT)
+    compiler->Bailout(ToCString());
+#endif  // defined(PRODUCT)
+    return NEXT_IS_FALSE;
+  }
+}
+
+
+EMIT_NATIVE_CODE(RelationalOp, 2, Location::RequiresRegister()) {
+  ASSERT(compiler->is_optimizing());
+  Label is_true, is_false;
+  BranchLabels labels = { &is_true, &is_false, &is_false };

[Vyacheslav Egorov (Google), 2016/07/01 16:15:35]

The same issue again. You are falling through to a true case while labels say
you are falling through to the false case.

[zra, 2016/07/01 17:43:45]

Acknowledged.

+  const Register result = locs()->out(0).reg();
+  __ LoadConstant(result, Bool::False());
+  Condition true_condition = EmitComparisonCode(compiler, labels);
+  ASSERT(true_condition == NEXT_IS_TRUE);
+  __ LoadConstant(result, Bool::True());
+}
+
+
+void RelationalOpInstr::EmitBranchCode(FlowGraphCompiler* compiler,
+                                       BranchInstr* branch) {
+  BranchLabels labels = compiler->CreateBranchLabels(branch);
+  Condition true_condition = EmitComparisonCode(compiler, labels);
+  EmitBranchOnCondition(compiler, true_condition, labels);
+}
+
+
+EMIT_NATIVE_CODE(CheckArrayBound, 2) {
+  const Register length = locs()->in(kLengthPos).reg();
+  const Register index = locs()->in(kIndexPos).reg();
+  __ IfLe(length, index);

[Vyacheslav Egorov (Google), 2016/07/01 16:15:35]

This is not correct. This does not check for negative index, code in other
back-ends is using *unsigned*.

[zra, 2016/07/01 17:43:45]

Do you have a preference whether we should have unsigned comparison
instructions, or add a flag to IfLe saying signed or unsigned?

[zra, 2016/07/01 20:55:39]

Added unsigned comparison instructions. We can revisit if we're running short on
instructions.

+  compiler->EmitDeopt(deopt_id(),
+                      ICData::kDeoptCheckArrayBound,
+                      (generalized_ ? ICData::kGeneralized : 0) |
+                      (licm_hoisted_ ? ICData::kHoisted : 0));
+}
+
 }  // namespace dart
 
 #endif  // defined TARGET_ARCH_DBC