Comments addressed
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());
On 2011/11/11 10:12:34, Kevin Millikin wrote:
Maybe I'm missing a good reason for this, but please change this to an
ASSERT.
Done (checked with Erik that we can actally make this an ASSERT again
(it was introduced in revision 9482 to catch a bug)
http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode134
src/code-stubs.cc:134: CHECK(AddToSpecialCache(new_object));
On 2011/11/11 10:12:34, Kevin Millikin wrote:
This function can only return true, so get rid of the CHECK and make
the return
type void.
Done.
http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode174
src/code-stubs.cc:174: Isolate* isolate = new_object->GetIsolate();
On 2011/11/11 10:12:34, Kevin Millikin wrote:
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);
}
I discussed with Erik and found a solution for using the code-cache (see
the new change in mark-compact.cc)
http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode194
src/code-stubs.cc:194: UNREACHABLE();
On 2011/11/11 10:12:34, Kevin Millikin wrote:
Huh?
Done.
http://codereview.chromium.org/8520006/diff/29001/src/code-stubs.cc#newcode228
src/code-stubs.cc:228: GenerateKnownObjects(masm, known_map_);
On 2011/11/11 10:12:34, Kevin Millikin wrote:
No real need to pass known_map_ here.
Done.
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);
On 2011/11/11 10:12:34, Kevin Millikin wrote:
It seems better to declare these virtual.
Done.
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) {
On 2011/11/11 10:12:34, Kevin Millikin wrote:
No need for the extra argument.
Done.
http://codereview.chromium.org/8520006/diff/29001/src/ia32/code-stubs-ia32.cc#newcode6514
src/ia32/code-stubs-ia32.cc:6514: __ cmp(ecx, map);
On 2011/11/11 10:12:34, Kevin Millikin wrote:
cmp(ecx, known_map_)
Done (and below)
http://codereview.chromium.org/8520006/diff/29001/src/ia32/code-stubs-ia32.cc#newcode6529
src/ia32/code-stubs-ia32.cc:6529: __ pop(ecx);
On 2011/11/11 10:12:34, Kevin Millikin wrote:
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.
Done.
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)) {
On 2011/11/11 10:12:34, Kevin Millikin wrote:
This is much nicer.
The last check can be written Token::IsEqualityOp(op_)
Done.
http://codereview.chromium.org/8520006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
