Next round of comments.
http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc File src/code-stubs.cc (right): http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode107 src/code-stubs.cc:107: CHECK(IsPregenerated() == code->is_pregenerated()); Maybe I'm missing a good reason for this, but please change this to an ASSERT. http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode134 src/code-stubs.cc:134: CHECK(AddToSpecialCache(new_object)); This function can only return true, so get rid of the CHECK and make the return type void. http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode174 src/code-stubs.cc:174: Isolate* isolate = new_object->GetIsolate(); I still don't fully understand why we need a separate caching mechanism here (but didn't before). I want to talk to the GC gurus a bit more. I'm worried that adding special caches doesn't scale. Could we instead add just a special symbol and use the cache in the map. The implementation would become something like: void ICCompareStub::AddToSpecialCache(Handle<Code> new_object) { return Map::UpdateCodeCache(known_map_, factory()->compare_ic_symbol(), new_object); } http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode194 src/code-stubs.cc:194: UNREACHABLE(); Huh? http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode228 src/code-stubs.cc:228: GenerateKnownObjects(masm, known_map_); No real need to pass known_map_ here. http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.h File src/code-stubs.h (right): http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.h#newcode474 src/code-stubs.h:474: bool AddToSpecialCache(Handle<Code> new_object); It seems better to declare these virtual. http://codereview.chromium.org/8520006/diff/29001/src/ia32/code-stubs-ia32.cc File src/ia32/code-stubs-ia32.cc (right): http://codereview.chromium.org/8520006/diff/29001/src/ia32/code-stubs-ia32.cc#newcode6506 src/ia32/code-stubs-ia32.cc:6506: Handle<Map> map) { No need for the extra argument. http://codereview.chromium.org/8520006/diff/29001/src/ia32/code-stubs-ia32.cc#newcode6514 src/ia32/code-stubs-ia32.cc:6514: __ cmp(ecx, map); cmp(ecx, known_map_) http://codereview.chromium.org/8520006/diff/29001/src/ia32/code-stubs-ia32.cc#newcode6529 src/ia32/code-stubs-ia32.cc:6529: __ pop(ecx); I'm not sure why this code is written this way, but since you're in this file, could you clean it up a bit: there is no reason to pop the return address to save eax and edx in the caller's frame. Just save them on the internal frame since there's going to be one anyway. http://codereview.chromium.org/8520006/diff/29001/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8520006/diff/29001/src/ic.cc#newcode2343 src/ic.cc:2343: (op_ == Token::EQ || op_ == Token::EQ_STRICT)) { This is much nicer. The last check can be written Token::IsEqualityOp(op_) http://codereview.chromium.org/8520006/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
