Second attempt: Improve our type feedback by recogizining never-executed IC...

Second attempt: Improve our type feedback by recogizining never-executed IC calls for binary operations.

This is an improved version of my earlier change r5970. It avoids degrading the
non-optimized code.

Initially we emit a conditional branch that is either always- or never-taken
after a smi-check (depending on whether we test for smi for for non-smi)
Since test-eax always sets the carry-flag to 0 we use jump-if-carry and
jump-if-not-carry.

The first invocation of the stub patches a jc with a jz and
jnc with a jnz-instruction so that the code looks exactly as it was
without patching. The only difference is the test- or nop-instruction
after the IC-call.

Review URL: http://codereview.chromium.org/5763004

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6030 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/arm/ic-arm.cc

@@ -2360,10 +2360,8 @@ Condition CompareIC::ComputeCondition(Token::Value op) {
 void CompareIC::UpdateCaches(Handle<Object> x, Handle<Object> y) {
   HandleScope scope;
   Handle<Code> rewritten;
-#ifdef DEBUG
   State previous_state = GetState();
-#endif
-  State state = TargetState(x, y);
+  State state = TargetState(previous_state, false, x, y);
   if (state == GENERIC) {
     CompareStub stub(GetCondition(), strict(), NO_COMPARE_FLAGS, r1, r0);
     rewritten = stub.GetCode();
@@ -2383,6 +2381,12 @@ void CompareIC::UpdateCaches(Handle<Object> x, Handle<Object> y) {
 #endif
 }
 
+
+void PatchInlinedSmiCode(Address address) {
+  UNIMPLEMENTED();
+}
+
+
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_ARM

src/full-codegen.h

@@ -38,6 +38,9 @@
 namespace v8 {
 namespace internal {
 
+// Forward declarations.
+class JumpPatchSite;
+
 // AST node visitor which can tell whether a given statement will be breakable
 // when the code is compiled by the full compiler in the debugger. This means
 // that there will be an IC (load/store/call) in the code generated for the
@@ -533,6 +536,10 @@ class FullCodeGenerator: public AstVisitor {
   // Helper for calling an IC stub.
   void EmitCallIC(Handle<Code> ic, RelocInfo::Mode mode);
 
+  // Calling an IC stub with a patch site. Passing NULL for patch_site
+  // indicates no inlined smi code and emits a nop after the IC call.
+  void EmitCallIC(Handle<Code> ic, JumpPatchSite* patch_site);
+
   // Set fields in the stack frame. Offsets are the frame pointer relative
   // offsets defined in, e.g., StandardFrameConstants.
   void StoreToFrameField(int frame_offset, Register value);

src/ia32/assembler-ia32.h

@@ -571,6 +571,15 @@ class Assembler : public Malloced {
   static const byte kTestEaxByte = 0xA9;
   // One byte opcode for test al, 0xXX.
   static const byte kTestAlByte = 0xA8;
+  // One byte opcode for nop.
+  static const byte kNopByte = 0x90;
+
+  // One byte opcode for a short unconditional jump.
+  static const byte kJmpShortOpcode = 0xEB;
+  // One byte prefix for a short conditional jump.
+  static const byte kJccShortPrefix = 0x70;
+  static const byte kJncShortOpcode = kJccShortPrefix | not_carry;
+  static const byte kJcShortOpcode = kJccShortPrefix | carry;
 
   // ---------------------------------------------------------------------------
   // Code generation

src/ia32/code-stubs-ia32.cc

@@ -1366,8 +1366,8 @@ void TypeRecordingBinaryOpStub::GenerateSmiCode(MacroAssembler* masm,
   if (op_ == Token::DIV || op_ == Token::MOD) {
     left = eax;
     right = ebx;
-      __ mov(ebx, eax);
-      __ mov(eax, edx);
+    __ mov(ebx, eax);
+    __ mov(eax, edx);
   }
 
 

src/ia32/code-stubs-ia32.h

@@ -250,7 +250,8 @@ class TypeRecordingBinaryOpStub: public CodeStub {
     ASSERT(OpBits::is_valid(Token::NUM_TOKENS));
   }
 
-  TypeRecordingBinaryOpStub(int key,
+  TypeRecordingBinaryOpStub(
+      int key,
       TRBinaryOpIC::TypeInfo operands_type,
       TRBinaryOpIC::TypeInfo result_type = TRBinaryOpIC::UNINITIALIZED)
       : op_(OpBits::decode(key)),
@@ -258,8 +259,7 @@ class TypeRecordingBinaryOpStub: public CodeStub {
         use_sse3_(SSE3Bits::decode(key)),
         operands_type_(operands_type),
         result_type_(result_type),
-        name_(NULL) {
-  }
+        name_(NULL) { }
 
   // Generate code to call the stub with the supplied arguments. This will add
   // code at the call site to prepare arguments either in registers or on the

src/ia32/ic-ia32.cc

@@ -2049,13 +2049,23 @@ Condition CompareIC::ComputeCondition(Token::Value op) {
 }
 
 
+static bool HasInlinedSmiCode(Address address) {
+  // The address of the instruction following the call.
+  Address test_instruction_address =
+      address + Assembler::kCallTargetAddressOffset;
+
+  // If the instruction following the call is not a test al, nothing
+  // was inlined.
+  return *test_instruction_address == Assembler::kTestAlByte;
+}
+
+
 void CompareIC::UpdateCaches(Handle<Object> x, Handle<Object> y) {
   HandleScope scope;
   Handle<Code> rewritten;
-#ifdef DEBUG
   State previous_state = GetState();
-#endif
-  State state = TargetState(x, y);
+
+  State state = TargetState(previous_state, HasInlinedSmiCode(address()), x, y);
   if (state == GENERIC) {
     CompareStub stub(GetCondition(), strict(), NO_COMPARE_FLAGS);
     rewritten = stub.GetCode();
@@ -2073,6 +2083,44 @@ void CompareIC::UpdateCaches(Handle<Object> x, Handle<Object> y) {
            Token::Name(op_));
   }
 #endif
+
+  // Activate inlined smi code.
+  if (previous_state == UNINITIALIZED) {
+    PatchInlinedSmiCode(address());
+  }
+}
+
+
+void PatchInlinedSmiCode(Address address) {
+  // The address of the instruction following the call.
+  Address test_instruction_address =
+      address + Assembler::kCallTargetAddressOffset;
+
+  // If the instruction following the call is not a test al, nothing
+  // was inlined.
+  if (*test_instruction_address != Assembler::kTestAlByte) {
+    ASSERT(*test_instruction_address == Assembler::kNopByte);
+    return;
+  }
+
+  Address delta_address = test_instruction_address + 1;
+  // The delta to the start of the map check instruction and the
+  // condition code uses at the patched jump.
+  int8_t delta = *reinterpret_cast<int8_t*>(delta_address);
+  if (FLAG_trace_ic) {
+    PrintF("[  patching ic at %p, test=%p, delta=%d\n",
+           address, test_instruction_address, delta);
+  }
+
+  // Patch with a short conditional jump. There must be a
+  // short jump-if-carry/not-carry at this position.
+  Address jmp_address = test_instruction_address - delta;
+  ASSERT(*jmp_address == Assembler::kJncShortOpcode ||
+         *jmp_address == Assembler::kJcShortOpcode);
+  Condition cc = *jmp_address == Assembler::kJncShortOpcode
+      ? not_zero
+      : zero;
+  *jmp_address = static_cast<byte>(Assembler::kJccShortPrefix | cc);
 }
 
 

src/ia32/lithium-codegen-ia32.cc

@@ -315,6 +315,13 @@ void LCodeGen::CallCode(Handle<Code> code,
     __ call(code, mode);
     RecordSafepoint(&no_pointers, Safepoint::kNoDeoptimizationIndex);
   }
+
+  // Signal that we don't inline smi code before these stubs in the
+  // optimizing code generator.
+  if (code->kind() == Code::TYPE_RECORDING_BINARY_OP_IC ||
+      code->kind() == Code::COMPARE_IC) {
+    __ nop();
+  }
 }
 
 

src/ic.cc

@@ -1951,7 +1951,7 @@ TRBinaryOpIC::State TRBinaryOpIC::ToState(TypeInfo type_info) {
 
 
 TRBinaryOpIC::TypeInfo TRBinaryOpIC::JoinTypes(TRBinaryOpIC::TypeInfo x,
-                                           TRBinaryOpIC::TypeInfo y) {
+                                               TRBinaryOpIC::TypeInfo y) {
   if (x == UNINITIALIZED) return y;
   if (y == UNINITIALIZED) return x;
   if (x == STRING && y == STRING) return STRING;
@@ -2041,6 +2041,11 @@ MaybeObject* TypeRecordingBinaryOp_Patch(Arguments args) {
              TRBinaryOpIC::GetName(result_type),
              Token::Name(op));
     }
+
+    // Activate inlined smi code.
+    if (previous_type == TRBinaryOpIC::UNINITIALIZED) {
+      PatchInlinedSmiCode(ic.address());
+    }
   }
 
   Handle<JSBuiltinsObject> builtins = Top::builtins();
@@ -2127,13 +2132,17 @@ const char* CompareIC::GetStateName(State state) {
 }
 
 
-CompareIC::State CompareIC::TargetState(Handle<Object> x, Handle<Object> y) {
-  State state = GetState();
-  if (state != UNINITIALIZED) return GENERIC;
-  if (x->IsSmi() && y->IsSmi()) return SMIS;
-  if (x->IsNumber() && y->IsNumber()) return HEAP_NUMBERS;
+CompareIC::State CompareIC::TargetState(State state,
+                                        bool has_inlined_smi_code,
+                                        Handle<Object> x,
+                                        Handle<Object> y) {
+  if (!has_inlined_smi_code && state != UNINITIALIZED) return GENERIC;
+  if (state == UNINITIALIZED && x->IsSmi() && y->IsSmi()) return SMIS;
+  if ((state == UNINITIALIZED || (state == SMIS && has_inlined_smi_code)) &&
+      x->IsNumber() && y->IsNumber()) return HEAP_NUMBERS;
   if (op_ != Token::EQ && op_ != Token::EQ_STRICT) return GENERIC;
-  if (x->IsJSObject() && y->IsJSObject()) return OBJECTS;
+  if (state == UNINITIALIZED &&
+      x->IsJSObject() && y->IsJSObject()) return OBJECTS;
   return GENERIC;
 }
 

src/ic.h

@@ -582,7 +582,8 @@ class CompareIC: public IC {
   static const char* GetStateName(State state);
 
  private:
-  State TargetState(Handle<Object> x, Handle<Object> y);
+  State TargetState(State state, bool has_inlined_smi_code,
+                    Handle<Object> x, Handle<Object> y);
 
   bool strict() const { return op_ == Token::EQ_STRICT; }
   Condition GetCondition() const { return ComputeCondition(op_); }
@@ -591,6 +592,8 @@ class CompareIC: public IC {
   Token::Value op_;
 };
 
+// Helper for TRBinaryOpIC and CompareIC.
+void PatchInlinedSmiCode(Address address);
 
 } }  // namespace v8::internal
 

src/type-info.cc

@@ -142,6 +142,9 @@ TypeInfo TypeFeedbackOracle::CompareType(CompareOperation* expr, Side side) {
   CompareIC::State state = static_cast<CompareIC::State>(code->compare_state());
   switch (state) {
     case CompareIC::UNINITIALIZED:
+      // Uninitialized means never executed.
+      // TODO(fschneider): Introduce a separate value for never-executed ICs.
+      return unknown;
     case CompareIC::SMIS:
       return TypeInfo::Smi();
     case CompareIC::HEAP_NUMBERS:
@@ -184,6 +187,9 @@ TypeInfo TypeFeedbackOracle::BinaryType(BinaryOperation* expr, Side side) {
 
     switch (type) {
       case TRBinaryOpIC::UNINITIALIZED:
+        // Uninitialized means never executed.
+        // TODO(fschneider): Introduce a separate value for never-executed ICs
+        return unknown;
       case TRBinaryOpIC::SMI:
         switch (result_type) {
           case TRBinaryOpIC::UNINITIALIZED:
@@ -224,6 +230,9 @@ TypeInfo TypeFeedbackOracle::SwitchType(CaseClause* clause) {
   CompareIC::State state = static_cast<CompareIC::State>(code->compare_state());
   switch (state) {
     case CompareIC::UNINITIALIZED:
+      // Uninitialized means never executed.
+      // TODO(fschneider): Introduce a separate value for never-executed ICs.
+      return unknown;
     case CompareIC::SMIS:
       return TypeInfo::Smi();
     case CompareIC::HEAP_NUMBERS:

src/x64/ic-x64.cc

@@ -1951,10 +1951,8 @@ Condition CompareIC::ComputeCondition(Token::Value op) {
 void CompareIC::UpdateCaches(Handle<Object> x, Handle<Object> y) {
   HandleScope scope;
   Handle<Code> rewritten;
-#ifdef DEBUG
   State previous_state = GetState();
-#endif
-  State state = TargetState(x, y);
+  State state = TargetState(previous_state, false, x, y);
   if (state == GENERIC) {
     CompareStub stub(GetCondition(), strict(), NO_COMPARE_FLAGS);
     rewritten = stub.GetCode();
@@ -1974,6 +1972,10 @@ void CompareIC::UpdateCaches(Handle<Object> x, Handle<Object> y) {
 #endif
 }
 
+void PatchInlinedSmiCode(Address address) {
+  UNIMPLEMENTED();
+}
+
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_X64