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

src/ic.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 2554 matching lines @@
                                        Token::Value op) {
   BinaryOpIC::TypeInfo new_type = TypeInfoFromValue(value, op);
   if (old_type == BinaryOpIC::STRING) {
     if (new_type == BinaryOpIC::STRING) return new_type;
     return BinaryOpIC::GENERIC;
   }
   return Max(old_type, new_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);
 
   HandleScope scope(isolate);
   Handle<Object> left = args.at<Object>(0);
   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);
 
   BinaryOpIC::TypeInfo new_left = InputState(previous_left, left, op);
   BinaryOpIC::TypeInfo new_right = InputState(previous_right, right, op);
   BinaryOpIC::TypeInfo result_type = BinaryOpIC::UNINITIALIZED;
 
   // STRING is only used for ADD operations.
   if ((new_left == BinaryOpIC::STRING || new_right == BinaryOpIC::STRING) &&
       op != Token::ADD) {
     new_left = new_right = BinaryOpIC::GENERIC;
   }
 
   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));
     }
 #endif
     BinaryOpIC ic(isolate);
     ic.patch(*code);
 
     // Activate inlined smi code.
     if (previous_overall == BinaryOpIC::UNINITIALIZED) {
@@ skipping 358 matching lines @@
 #undef ADDR
 };
 
 
 Address IC::AddressFromUtilityId(IC::UtilityId id) {
   return IC_utilities[id];
 }
 
 
 } }  // namespace v8::internal