Speed up Binops

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 3a1598431f1057ef0ceda75572875ccec50b166d..e90eeae5c47af47773b9c68c463860f25b14014b 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -3122,7 +3122,7 @@ void HGraph::RestoreActualValues() {
 }
 
 
-void HOptimizedGraphBuilder::PushAndAdd(HInstruction* instr) {
+void HGraphBuilder::PushAndAdd(HInstruction* instr) {
   Push(instr);
   AddInstruction(instr);
 }
@@ -7852,7 +7852,7 @@ static bool ShiftAmountsAllowReplaceByRotate(HValue* sa,
 // directions that can be replaced by one rotate right instruction or not.
 // Returns the operand and the shift amount for the rotate instruction in the
 // former case.
-bool HOptimizedGraphBuilder::MatchRotateRight(HValue* left,
+bool HGraphBuilder::MatchRotateRight(HValue* left,
                                               HValue* right,
                                               HValue** operand,
                                               HValue** shift_amount) {
@@ -7891,7 +7891,24 @@ bool CanBeZero(HValue* right) {
 }
 
 
-HInstruction* HOptimizedGraphBuilder::BuildBinaryOperation(
+HValue* HGraphBuilder::ConstantToNumber(HValue* value) {

[Toon Verwaest, 2013/07/18 13:57:12]

Perhaps you can also handle strings here.

+  if (value->IsConstant()) {

[Toon Verwaest, 2013/07/18 13:57:12]

Move this code together with HConstant::CopyToTruncatedInt32 as "CopyToDouble",
and perhaps extend CopyToTruncatedInt32 to also directly handle the special
values handled below.

+    HConstant* constant = HConstant::cast(value);
+    if (constant->handle()->IsBoolean()) {
+      return constant->handle()->BooleanValue()
+        ? graph()->GetConstant1() : graph()->GetConstant0();

[Jakob Kummerow, 2013/07/18 11:56:15]

nit: indentation

+    } else if (constant->handle()->IsUndefined() ||
+               constant->handle()->IsTheHole()) {

[Toon Verwaest, 2013/07/18 13:57:12]

Remove TheHole case.

+      return Add<HConstant>(OS::nan_value(), Representation::Double());
+    } else if (constant->handle()->IsNull()) {
+      return graph()->GetConstant0();
+    }
+  }
+  return value;
+}
+
+
+HInstruction* HGraphBuilder::BuildBinaryOperation(
     BinaryOperation* expr,
     HValue* left,
     HValue* right) {
@@ -7900,9 +7917,15 @@ HInstruction* HOptimizedGraphBuilder::BuildBinaryOperation(
   Handle<Type> right_type = expr->right()->bounds().lower;
   Handle<Type> result_type = expr->bounds().lower;
   Maybe<int> fixed_right_arg = expr->fixed_right_arg();
-  Representation left_rep = Representation::FromType(left_type);
-  Representation right_rep = Representation::FromType(right_type);
-  Representation result_rep = Representation::FromType(result_type);
+
+  if (expr->op() != Token::ADD ||
+      (left->type().IsNoString() && right->type().IsNoString())) {
+    left = ConstantToNumber(left);
+    right = ConstantToNumber(right);
+  }
+
+  left_type = left->type().intersect(left_type, isolate());
+  right_type = right->type().intersect(right_type, isolate());
 
   if (left_type->Is(Type::None())) {
     AddSoftDeoptimize();
@@ -7913,14 +7936,23 @@ HInstruction* HOptimizedGraphBuilder::BuildBinaryOperation(
     AddSoftDeoptimize();
     right_type = handle(Type::Any(), isolate());
   }
+
+  Representation left_rep = left->IsConstant() ?
+    left->KnownOptimalRepresentation() : Representation::FromType(left_type);

[Jakob Kummerow, 2013/07/18 11:56:15]

nit: indentation. Also, prefer the '?' on the next line. Maybe:
  Representation left_rep = left->IsConstant()
      ? left->KnownOptimalRepresentation()
      : Representation::FromType(left_type);

[Toon Verwaest, 2013/07/18 13:57:12]

Could we calculate this in "type" and only at the end convert to Representation?
Now it's pretty schizophrenic mixing HType, Type and Representation.

+  Representation right_rep = right->IsConstant() ?
+    right->KnownOptimalRepresentation() : Representation::FromType(right_type);
+  Representation result_rep = Representation::FromType(result_type);
+
   HInstruction* instr = NULL;
   switch (expr->op()) {
     case Token::ADD:
       if (left_type->Is(Type::String()) && right_type->Is(Type::String())) {
-        BuildCheckHeapObject(left);
-        AddInstruction(HCheckInstanceType::NewIsString(left, zone()));
-        BuildCheckHeapObject(right);
-        AddInstruction(HCheckInstanceType::NewIsString(right, zone()));
+        if (!left->type().IsString() || !right->type().IsString()) {

[Jakob Kummerow, 2013/07/18 11:56:15]

Why is this one atomic block rather than "if (!left is string) { add check for
left; }" and then the same for |right|?

+          BuildCheckHeapObject(left);
+          AddInstruction(HCheckInstanceType::NewIsString(left, zone()));
+          BuildCheckHeapObject(right);
+          AddInstruction(HCheckInstanceType::NewIsString(right, zone()));

[Toon Verwaest, 2013/07/18 13:57:12]

I'd use canonicalization to eliminate CheckInstanceType(string) of values that
are known to be type().IsString(); rather than conditionally emit the
instructions.

+        }
         instr = HStringAdd::New(zone(), context, left, right);
       } else {
         instr = HAdd::New(zone(), context, left, right);