Introduce extra IC state to record additional feedback from IC-s.

Introduce extra IC state to record additional feedback from IC-s.

Extra IC state is only two bits and only supported for call IC-s for
now. To change its extra state an IC stub jumps to a new miss stub
that goes to runtime as usual but then instead of going megamorphic
generates a monomorphic stub with the updated state.

Committed: http://code.google.com/p/v8/source/detail?r=6370

[Mads Ager (chromium), 2011-01-17 14:06:00]

http://codereview.chromium.org/6344005/diff/3001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6344005/diff/3001/src/ic.cc#newcode1651
src/ic.cc:1651: if (args.length() == 2) {
As discussed offline I would like to keep all the patching decisions in the
runtime system if possible. Based on receiver, name and target code stub we
should be able to make the decision without extra arguments to the miss code.
This would keep generated code simple: if you can't handle it, just call miss.
And we should have all the flexibility needed in the runtime system.

[Vitaly Repeshko, 2011-01-17 18:02:36]

Moved the logic to the runtime system. The generated code is simpler now. Ready
for another look.


Thanks,
Vitaly

[Mads Ager (chromium), 2011-01-18 11:28:57]

This patch confuses me. You went back to one miss stub that takes just two
arguments. However, in the runtime system you still check for the additional
argument. Am I missing something?

http://codereview.chromium.org/6344005/diff/6001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6344005/diff/6001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:1367: extra_ic_state_);
Why do you need the extra_ic_state_ here? One miss stub per kind should be
enough?

http://codereview.chromium.org/6344005/diff/6001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6344005/diff/6001/src/ic.cc#newcode657
src/ic.cc:657: if (kind_ == Code::CALL_IC && state == MONOMORPHIC) {
Why isn't this just part of the normal MONOMORPHIC branch?

http://codereview.chromium.org/6344005/diff/6001/src/ic.cc#newcode1719
src/ic.cc:1719: ASSERT(args.length() == 2 || args.length() == 3);
Why do you need to pass the extra state to the miss call at all? We should have
enough info to figure this out in the runtime system. Is this a left over from
the previous change?

[Vitaly Repeshko, 2011-01-18 11:52:07]

Removed the forgotten parts of the old code. Sorry.

[Vitaly Repeshko, 2011-01-18 12:13:46]

http://codereview.chromium.org/6344005/diff/6001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6344005/diff/6001/src/ic.cc#newcode657
src/ic.cc:657: if (kind_ == Code::CALL_IC && state == MONOMORPHIC) {
On 2011/01/18 11:28:57, Mads Ager wrote:
> Why isn't this just part of the normal MONOMORPHIC branch?

Because we change the state to PREMONORPHIC to generate the right stub.

[Mads Ager (chromium), 2011-01-18 12:54:00]

I don't like the changing of the state to PREMONOMORPHIC much. That seems like a
very indirect way to get the computation of a monomorphic stub.

Other than that, LGTM.

http://codereview.chromium.org/6344005/diff/17001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6344005/diff/17001/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:1337: extra_ic_state_);
Needs updating to follow ia32.

http://codereview.chromium.org/6344005/diff/17001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6344005/diff/17001/src/ic.cc#newcode657
src/ic.cc:657: if (kind_ == Code::CALL_IC && state == MONOMORPHIC) {
This is non-trivial and requires a comment. 

Would it be clearer to not change the state to PREMONOMORPHIC here and handle
this as part of the MONOMORPHIC case below? Refactor to get a method to generate
monomorphic stubs and call it from the PREMONOMORPHIC and the MONOMORPHIC cases
below?

http://codereview.chromium.org/6344005/diff/17001/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/6344005/diff/17001/src/type-info.cc#newcode125
src/type-info.cc:125: Code::kNoExtraICState,
This mean that we cannot get type feedback for the receiver if there have been
out of bounds reads? That could still be useful, couldn't it? At least I think
it requires a comment.

http://codereview.chromium.org/6344005/diff/17001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/6344005/diff/17001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:1332: extra_ic_state_);
Needs updating to follow ia32.

[Mads Ager (chromium), 2011-01-18 15:32:26]

LGTM once x64 and arm code is updated.

http://codereview.chromium.org/6344005/diff/32001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6344005/diff/32001/src/ic.cc#newcode760
src/ic.cc:760: if (kind_ == Code::CALL_IC &&
Remove the extra indentation by doing:

if () {
  ...
} else if() {
  ...
} else {
  ...
}
?

[Mads Ager (chromium), 2011-01-18 15:50:55]

LGTM