CallIC must update type feedback info correctly.

CallIC must update type feedback info correctly.

CallIC, as the first of vector-based ICs didn't update the ic with type
count counter and the generic count counter correctly. This CL fixes
that.

BUG=
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22962

[mvstanton, 2014-08-06 13:11:59]

Hi Jakob,
Here is the CL we discussed. 
1) It duplicates too much code. Thinking about fixes.
2) I had to remove an optimization that went from MONOMORPHIC->GENERIC state
without missing to the runtime, because the stats need to be updated on that
transition.
3) There is a "test" to verify functionality, but I won't check this in, it's
just for you to see.

thx,
--Michael

[Jakob Kummerow, 2014-08-06 14:49:09]

Looking good. Some comments.

https://codereview.chromium.org/445943002/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (left):

https://codereview.chromium.org/445943002/diff/1/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:2459: // We are going megamorphic. If the feedback
is a JSFunction, it is fine
Removing optimizations is sad... is the type feedback vector for calls shared
with optimized code (when it's megamorphic)? Maybe we can just keep this code --
even though it violates consistency of overall system behavior, maybe it's an
exception to the rule that we can afford not to care about.

https://codereview.chromium.org/445943002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/445943002/diff/1/src/ic.cc#newcode1963
src/ic.cc:1963: void CallIC::UpdateTypeFeedbackInfo(Object* old_feedback,
Object* new_feedback) {
nit: 80col

https://codereview.chromium.org/445943002/diff/1/src/ic.cc#newcode1964
src/ic.cc:1964: // Convert old_feedback to an IC_State enum:
This comment seems to belong to line 1971, but I don't think it adds much value,
so feel free to just drop it.

https://codereview.chromium.org/445943002/diff/1/src/ic.cc#newcode1965
src/ic.cc:1965: Code* host = isolate()->
Idea for reducing code duplication: pull out most of this method (except for
lines 1971, 1972) into a helper with signature (Address*, IC::State old_state,
IC::State new_state).

The drawback is that you'll have to compute the states before you know whether
you'll need them; but that should be fairly cheap, and at least some of the
computations can be avoided: callers of this method know the new_state in many
(all?) cases, e.g. for vector->set(...MegamorphicSentinel...).

[mvstanton, 2014-08-06 15:32:33]

Thx for the comments, here is an update.

I removed the test. As noted, with the optimized switch to GENERIC without
missing to the runtime, we will underreport generic and overreport polymorphic.
That is reflected in my simple test.

thx!
--Michael

https://codereview.chromium.org/445943002/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (left):

https://codereview.chromium.org/445943002/diff/1/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:2459: // We are going megamorphic. If the feedback
is a JSFunction, it is fine
On 2014/08/06 14:49:09, Jakob wrote:
> Removing optimizations is sad... is the type feedback vector for calls shared
> with optimized code (when it's megamorphic)? Maybe we can just keep this code
--
> even though it violates consistency of overall system behavior, maybe it's an
> exception to the rule that we can afford not to care about.

That sounds good to me. I'll move this change to a separate CL which I'll test
against benchmarks.

https://codereview.chromium.org/445943002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/445943002/diff/1/src/ic.cc#newcode1963
src/ic.cc:1963: void CallIC::UpdateTypeFeedbackInfo(Object* old_feedback,
Object* new_feedback) {
On 2014/08/06 14:49:09, Jakob wrote:
> nit: 80col

Done.

https://codereview.chromium.org/445943002/diff/1/src/ic.cc#newcode1964
src/ic.cc:1964: // Convert old_feedback to an IC_State enum:
On 2014/08/06 14:49:09, Jakob wrote:
> This comment seems to belong to line 1971, but I don't think it adds much
value,
> so feel free to just drop it.

Done.

https://codereview.chromium.org/445943002/diff/1/src/ic.cc#newcode1965
src/ic.cc:1965: Code* host = isolate()->
On 2014/08/06 14:49:09, Jakob wrote:
> Idea for reducing code duplication: pull out most of this method (except for
> lines 1971, 1972) into a helper with signature (Address*, IC::State old_state,
> IC::State new_state).
> 
> The drawback is that you'll have to compute the states before you know whether
> you'll need them; but that should be fairly cheap, and at least some of the
> computations can be avoided: callers of this method know the new_state in many
> (all?) cases, e.g. for vector->set(...MegamorphicSentinel...).

Excellent idea, done. I think the cost of computing a state var is very cheap.

[Jakob Kummerow, 2014-08-06 16:04:30]

lgtm

[mvstanton, 2014-08-07 08:52:08]

Committed patchset #2 manually as 22962 (presubmit successful).