Slight cleanup/unification of UpdateCache code.

Slight cleanup of UpdateCache code.

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

[Toon Verwaest, 2013-01-04 15:06:29]

PTAL

[Toon Verwaest, 2013-01-04 15:36:50]

Removed POLYMORPHIC code from this CL, PTAL again.

[Toon Verwaest, 2013-01-07 12:09:52]

PTAL

[ulan, 2013-01-07 15:04:53]

Looks good overall, few comments:

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc
File src/ic.cc (left):

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#oldcode2061
src/ic.cc:2061: JSFunction* raw_function = NULL;
I am not sure, might be that we need it to avoid compiler warnings on some
platform.

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode755
src/ic.cc:755: Map* map = target()->FindFirstMap();
Can we add a comment here like in LoadIC case?

Any reason why we use different checks here (code->ic_state() != MONOMORPHIC)
and in LoadIC (target() != *code) ?

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode1063
src/ic.cc:1063: isolate()->stub_cache()->Set(*name, receiver->map(), *code);
missing break.

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode1330
src/ic.cc:1330: }
missing break.

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode1593
src/ic.cc:1593: isolate()->stub_cache()->Set(*name, map, target());
Can we add a comment explaining this stub cache updates like in LoadIC case?

[Toon Verwaest, 2013-01-07 15:28:45]

Addressed comments, ptal

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc
File src/ic.cc (left):

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#oldcode2061
src/ic.cc:2061: JSFunction* raw_function = NULL;
I doubt it. It would be the only maybe->To(&value) site to require this... There
are many different legacy versions of this idiom lying around the codebase; I'm
just cleaning them up as I see them ;-)

On 2013/01/07 15:04:54, ulan wrote:
> I am not sure, might be that we need it to avoid compiler warnings on some
> platform.

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode755
src/ic.cc:755: Map* map = target()->FindFirstMap();
The code above calculates the target different from the other code. It may
update the monomorphic stub, thus staying monomorphic. It will already assign
code to be a megamorphic stub if it needs to become megamorphic in the code
above. Hence, we only update the cache if that code has decided to become
megamorphic.

On 2013/01/07 15:04:54, ulan wrote:
> Can we add a comment here like in LoadIC case?
> 
> Any reason why we use different checks here (code->ic_state() != MONOMORPHIC)
> and in LoadIC (target() != *code) ?

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode1063
src/ic.cc:1063: isolate()->stub_cache()->Set(*name, receiver->map(), *code);
On 2013/01/07 15:04:54, ulan wrote:
> missing break.

Done.

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode1330
src/ic.cc:1330: }
On 2013/01/07 15:04:54, ulan wrote:
> missing break.

Done.

https://chromiumcodereview.appspot.com/11737033/diff/2001/src/ic.cc#newcode1593
src/ic.cc:1593: isolate()->stub_cache()->Set(*name, map, target());
On 2013/01/07 15:04:54, ulan wrote:
> Can we add a comment explaining this stub cache updates like in LoadIC case?

Done.

[ulan, 2013-01-07 15:33:09]

LGTM