Covered all Anton's comments except the idea about api tests (need to
discuss)
http://codereview.chromium.org/2280007/diff/72002/86010
File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/2280007/diff/72002/86010#newcode1080
src/ia32/ic-ia32.cc:1080: __ CmpInstanceType(ebx, FIRST_NONSTRING_TYPE);
Exactly. As ebx contains the objects map was always above, so a
secondary probe was always skipped.
On 2010/06/03 16:18:06, antonm wrote:
I believe it was a bug we discussed with you?
http://codereview.chromium.org/2280007/diff/72002/86010#newcode1100
src/ia32/ic-ia32.cc:1100: __ bind(&miss);
Done. Added a comment that the generated code can fall through if both
probes miss.
On 2010/06/03 16:18:06, antonm wrote:
maybe pass miss label as an argument?
I think it would make it easier to use this function and looks more
v8ish imho.
http://codereview.chromium.org/2280007/diff/72002/86010#newcode1200
src/ia32/ic-ia32.cc:1200: __ bind(&miss);
Added a label parameter. Commented that this code never falls through.
On 2010/06/03 16:18:06, antonm wrote:
ditto
http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302
src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc,
Code::KEYED_CALL_IC);
There are many reasons for monomorphic cache to miss even for a
perfectly good key.
I do not see a reason to test KeyedLoadIC_Generic as it is used widely
across v8 and I am sure it is tested thoroughly.
Renamed the label to 'skip_probe'.
On 2010/06/03 16:18:06, antonm wrote:
isn't there is a problem if GenerateMonomorphicCacheProbe would jump
to miss
label? In that case you would take smi_key branch, correct? I think
that key
should be something like null or undefined to hit this path.
I believe (but I would appreciate if you add tests for that) that
KeyedLoadIC_Generic could cope with such keys, but maybe it should be
made more
explicit in the code (e.g. rename the label)
http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302
src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc,
Code::KEYED_CALL_IC);
If the stub lookup failed we fall through and call KeyedLoadIC_Generic.
This is the most sensible and fast thing to do, and there is absolutely
no reason to go into the miss case.
On 2010/06/03 16:18:06, antonm wrote:
similar note: it doesn't look like we go in miss case if we failed to
lookup a
stub for the name, correct?
http://codereview.chromium.org/2280007/diff/72002/86010#newcode1325
src/ia32/ic-ia32.cc:1325: __ j(below, &miss, not_taken);
Some of these checks are redundant, but I have a plan to get rid of them
in the next patch. There I will inline KeyedLoadIC_Generic code and
redirect its bailouts to bypass this code (I already have a rough
prototype).
On 2010/06/03 16:18:06, antonm wrote:
cache probing does some of these checks, I wonder if we could unify
them, but
that's probably too much of trouble
http://codereview.chromium.org/2280007/diff/72002/86011
File src/ic.cc (right):
http://codereview.chromium.org/2280007/diff/72002/86011#newcode605
src/ic.cc:605: } else {
On 2010/06/03 16:18:06, antonm wrote:
not insisting, but I'd rather omit the else-clause as you quite from
then-clause
quickly anyway
Done.
http://codereview.chromium.org/2280007/diff/72002/86011#newcode1390
src/ic.cc:1390: // Used from ic_<arch>.cc.
Fixed everywhere.
On 2010/06/03 16:18:06, antonm wrote:
Lie! used from ic-<arch>.cc :)
http://codereview.chromium.org/2280007/diff/72002/86012
File src/ic.h (right):
http://codereview.chromium.org/2280007/diff/72002/86012#newcode217
src/ic.h:217: class CallIC: public CallICBase {
We only ever construct these objects on stack, so the type is known at
compile time, no risk to mess things up. In fact, we hardly ever
heap-allocate C++ objects in v8.
On 2010/06/03 16:18:06, antonm wrote:
I know we already inherit from IC and there is no CallIC/KeyedCallIC
specific
fields, but still when I see growing hierarchy w/o virtual
destructors, I am
somewhat uneasy
http://codereview.chromium.org/2280007/diff/72002/86013
File src/stub-cache.cc (right):
http://codereview.chromium.org/2280007/diff/72002/86013#newcode445
src/stub-cache.cc:445: (kind == Code::CALL_IC ? Logger::type:
Logger::KEYED_##type)
On 2010/06/03 16:18:06, antonm wrote:
nit: a space before : (coming from ? : construct)
Done.
http://codereview.chromium.org/2280007/diff/72002/86029
File test/cctest/test-api.cc (right):
http://codereview.chromium.org/2280007/diff/72002/86029#newcode7126
test/cctest/test-api.cc:7126: "proto1 = new Object();"
On 2010/06/03 16:18:06, antonm wrote:
please, rename proto1 to o and o to proto1 to have o -> proto1 ->
proto2 chain
Done.
http://codereview.chromium.org/2280007/diff/72002/86029#newcode7144
test/cctest/test-api.cc:7144: // global object which is between
interceptor and constant function' holders.
Forgot to change the comment.
I am not quite getting your idea about adding another test, let's
discuss it.
On 2010/06/03 16:18:06, antonm wrote:
am I missing something, I don't see any override here.
Overall, it might be good to add a test case for two paths to miss
case: when
prototype chain changes from receiver to interceptor holder and from
interceptor
holder to cached lookup holder---we need to be sure correct name is
passed. But
yes, it should be the case.
http://codereview.chromium.org/2280007/diff/72002/86030
File test/mjsunit/keyed-call-ic.js (right):
http://codereview.chromium.org/2280007/diff/72002/86030#newcode158
test/mjsunit/keyed-call-ic.js:158: function testTypeTransitions() {
Oops. Forgot about that!
On 2010/06/03 16:18:06, antonm wrote:
why it's empty?
http://codereview.chromium.org/2280007/diff/72002/86030#newcode161
test/mjsunit/keyed-call-ic.js:161: testTypeTransitions();
On 2010/06/03 16:18:06, antonm wrote:
I think you should add a test for receiver changing its type.
Done.
http://codereview.chromium.org/2280007/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
