Collect type feedback for power-of-2 right operands in BinaryOps.

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index ea0c1fbbe1d70438ee2983ee0d682ef5d3700f24..22a88a5be8707adfc31152c5ddbdb24658dc6da0 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -2572,6 +2572,22 @@ static BinaryOpIC::TypeInfo InputState(BinaryOpIC::TypeInfo old_type,
 }
 
 
+static bool TryToInt32(Handle<Object> object, int32_t* result) {
+  if (object->IsSmi()) {
+    *result = Smi::cast(*object)->value();
+    return true;
+  }
+  if (object->IsHeapNumber()) {
+    double value = HeapNumber::cast(*object)->value();
+    if (TypeInfo::IsInt32Double(value)) {
+      *result = static_cast<int32_t>(value);
+      return true;
+    }
+  }
+  return false;
+}
+
+
 RUNTIME_FUNCTION(MaybeObject*, BinaryOp_Patch) {
   ASSERT(args.length() == 3);
 
@@ -2580,6 +2596,21 @@ RUNTIME_FUNCTION(MaybeObject*, BinaryOp_Patch) {
   Handle<Object> right = args.at<Object>(1);
   int key = args.smi_at(2);
   Token::Value op = BinaryOpStub::decode_op_from_minor_key(key);
+
+  bool previous_has_fixed_right_arg =
+      BinaryOpStub::decode_has_fixed_right_arg_from_minor_key(key);
+  int previous_fixed_right_arg_value =
+      BinaryOpStub::decode_fixed_right_arg_value_from_minor_key(key);
+
+  int32_t value;
+  bool new_has_fixed_right_arg =
+      op == Token::MOD &&
+      TryToInt32(right, &value) &&
+      BinaryOpStub::can_encode_arg_value(value) &&
+      (!previous_has_fixed_right_arg ||

[Jakob Kummerow, 2013/05/23 12:05:43]

I still disagree with this condition, as discussed offline previously. It should
be:
... &&
(previous_overall == BinaryOpIC::UNINITIALIZED ||
 (previous_has_fixed_right_arg && previous_fixed_right_arg_value == value));
Otherwise you'll go back to assuming a fixed right arg whenever the right arg
happens to be a power of two at a patching event even if you've seen non-fixed
right args before.

[Sven Panne, 2013/05/23 14:39:20]

Ooops, forgot that. Fixed.

+       previous_fixed_right_arg_value == value);
+  int32_t new_fixed_right_arg_value = new_has_fixed_right_arg ? value : 1;
+
   BinaryOpIC::TypeInfo previous_left, previous_right, unused_previous_result;
   BinaryOpStub::decode_types_from_minor_key(
       key, &previous_left, &previous_right, &unused_previous_result);
@@ -2597,40 +2628,51 @@ RUNTIME_FUNCTION(MaybeObject*, BinaryOp_Patch) {
   BinaryOpIC::TypeInfo new_overall = Max(new_left, new_right);
   BinaryOpIC::TypeInfo previous_overall = Max(previous_left, previous_right);
 
-  if (new_overall == BinaryOpIC::SMI && previous_overall == BinaryOpIC::SMI) {
-    if (op == Token::DIV ||
-        op == Token::MUL ||
-        op == Token::SHR ||
-        kSmiValueSize == 32) {
-      // Arithmetic on two Smi inputs has yielded a heap number.
-      // That is the only way to get here from the Smi stub.
-      // With 32-bit Smis, all overflows give heap numbers, but with
-      // 31-bit Smis, most operations overflow to int32 results.
-      result_type = BinaryOpIC::NUMBER;
-    } else {
-      // Other operations on SMIs that overflow yield int32s.
-      result_type = BinaryOpIC::INT32;
+  if (previous_has_fixed_right_arg == new_has_fixed_right_arg) {
+    if (new_overall == BinaryOpIC::SMI && previous_overall == BinaryOpIC::SMI) {
+      if (op == Token::DIV ||
+          op == Token::MUL ||
+          op == Token::SHR ||
+          kSmiValueSize == 32) {
+        // Arithmetic on two Smi inputs has yielded a heap number.
+        // That is the only way to get here from the Smi stub.
+        // With 32-bit Smis, all overflows give heap numbers, but with
+        // 31-bit Smis, most operations overflow to int32 results.
+        result_type = BinaryOpIC::NUMBER;
+      } else {
+        // Other operations on SMIs that overflow yield int32s.
+        result_type = BinaryOpIC::INT32;
+      }
     }
-  }
-  if (new_overall == BinaryOpIC::INT32 &&
-      previous_overall == BinaryOpIC::INT32) {
-    if (new_left == previous_left && new_right == previous_right) {
-      result_type = BinaryOpIC::NUMBER;
+    if (new_overall == BinaryOpIC::INT32 &&
+        previous_overall == BinaryOpIC::INT32) {
+      if (new_left == previous_left && new_right == previous_right) {
+        result_type = BinaryOpIC::NUMBER;
+      }
     }
   }
 
-  BinaryOpStub stub(key, new_left, new_right, result_type);
+  BinaryOpStub stub(key, new_left, new_right, result_type,
+                    new_has_fixed_right_arg, new_fixed_right_arg_value);
   Handle<Code> code = stub.GetCode(isolate);
   if (!code.is_null()) {
 #ifdef DEBUG
     if (FLAG_trace_ic) {
       PrintF("[BinaryOpIC in ");
       JavaScriptFrame::PrintTop(isolate, stdout, false, true);
-      PrintF(" ((%s+%s)->((%s+%s)->%s))#%s @ %p]\n",
+      PrintF(" ((%s+%s",

[Sven Panne, 2013/05/23 14:39:20]

While debugging this, I cleaned this trace output up a bit (and the one for
unary ops) to be more uniform and giving it ML-syntax. ;-)

              BinaryOpIC::GetName(previous_left),
-             BinaryOpIC::GetName(previous_right),
+             BinaryOpIC::GetName(previous_right));
+      if (previous_has_fixed_right_arg) {
+        PrintF("{%d}", previous_fixed_right_arg_value);
+      }
+      PrintF(")->((%s+%s",
              BinaryOpIC::GetName(new_left),
-             BinaryOpIC::GetName(new_right),
+             BinaryOpIC::GetName(new_right));
+      if (new_has_fixed_right_arg) {
+        PrintF("{%d}", new_fixed_right_arg_value);
+      }
+      PrintF(")->%s))#%s @ %p]\n",
              BinaryOpIC::GetName(result_type),
              Token::Name(op),
              static_cast<void*>(*code));