Polymorphism support for load IC.

Polymorphism support for load IC.

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

[Toon Verwaest, 2013-02-27 15:48:59]

PTAL. Only look at ia32 for now.

[Jakob Kummerow, 2013-03-01 22:21:04]

Nice work!

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/code-stubs.h
File src/code-stubs.h (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/code-stubs.h#ne...
src/code-stubs.h:672: : HandlerStub(Code::LOAD_IC), reg_(reg),
nit: let's give each initializer statement its own line.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/code-stubs.h#ne...
src/code-stubs.h:680: class RegisterBits: public BitField<int, 1, 4> {};
Looks like something's wrong with those bit fields. The meaning of the template
arguments is "type, shift, size". ICStub::MinorKey() needs 4 bits. So you
probably want:
CodeKindBits -> int, 0, 4 (I'd make this explicit for clarity, and for getting
ASSERT coverage; use "CodeKindBits::encode(HandlerStub::MinorKey())" below.)
RegisterBits -> int, 4, 4 (is 4 bits enough on all platforms?)
InobjectBits -> bool, 8, 1
IndexBits -> int, 9, 16 (is there a reasonable upper limit for |index_|? If yes,
use that; otherwise 16 == kStubMinorKeyBits - 9).

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.cc#newcode1027
src/ic.cc:1027: UpdateMegamorphicCache(map, *name, target());
Don't you want to do this for load stubs too when UpdatePolymorphicIC() has
returned false? (Similar to how you do it in line 1050.)

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.cc#newcode1055
src/ic.cc:1055: // When trying to patch a polymorphic keyed load/store element
stub
Comment seems inaccurate: We end up here when we try to patch anything other
than a polymorphic load stub.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.h
File src/ic.h (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.h#newcode170
src/ic.h:170: Handle<String> name) {
I find it slightly less than ideal that we have a mix of "object, name, code"
and "object, code, name" method signatures, but I'm finding it hard to decide
which is better. The former is motivated by the purpose of the IC system being
to compute a (receiver × name → handler) function, the latter makes more sense
for helper functions taking a map list and a code list that must be exactly in
sync with each other. Maybe migrating everything to "name, object, code" would
be a compromise? In a separate clean-up CL? Or we could just leave it and worry
about more important things :-)

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.cc#n...
src/stub-cache.cc:110: 
nit: two empty lines

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.cc#n...
src/stub-cache.cc:1390: Handle<JSObject> object,
Why the reformatting?

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.h
File src/stub-cache.h (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.h#ne...
src/stub-cache.h:628: Handle<JSObject> object,
Why this reformatting?

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.h#ne...
src/stub-cache.h:641: Handle<JSObject> object,
Why this reformatting?

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/type-info.cc
File src/type-info.cc (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/type-info.cc#ne...
src/type-info.cc:506: types->Add(maps.at(i), zone());
Please compare this with CollectKeyedReceiverTypes() below. In particular,
you'll want the !CanRetainOtherContext() check here as well.
Bonus points for more unification of the two methods (use FindAllMaps below? Or
factor out the code object walking into yet another helper function that fills a
SmallMapList* directly and does the context-retaining check?), but that's
optional ;-)

[Toon Verwaest, 2013-03-04 10:54:08]

Addressed comments

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/code-stubs.h
File src/code-stubs.h (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/code-stubs.h#ne...
src/code-stubs.h:672: : HandlerStub(Code::LOAD_IC), reg_(reg),
On 2013/03/01 22:21:04, Jakob wrote:
> nit: let's give each initializer statement its own line.

Done.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/code-stubs.h#ne...
src/code-stubs.h:680: class RegisterBits: public BitField<int, 1, 4> {};
Doh, what did I do there.

2**11 is a reasonable upper limit for fast properties, given that that's the max
number of descriptors we support.

I used 6 bits for the registers as reasonable upper limit.

On 2013/03/01 22:21:04, Jakob wrote:
> Looks like something's wrong with those bit fields. The meaning of the
template
> arguments is "type, shift, size". ICStub::MinorKey() needs 4 bits. So you
> probably want:
> CodeKindBits -> int, 0, 4 (I'd make this explicit for clarity, and for getting
> ASSERT coverage; use "CodeKindBits::encode(HandlerStub::MinorKey())" below.)
> RegisterBits -> int, 4, 4 (is 4 bits enough on all platforms?)
> InobjectBits -> bool, 8, 1
> IndexBits -> int, 9, 16 (is there a reasonable upper limit for |index_|? If
yes,
> use that; otherwise 16 == kStubMinorKeyBits - 9).

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.cc#newcode1027
src/ic.cc:1027: UpdateMegamorphicCache(map, *name, target());
On 2013/03/01 22:21:04, Jakob wrote:
> Don't you want to do this for load stubs too when UpdatePolymorphicIC() has
> returned false? (Similar to how you do it in line 1050.)

Done.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/ic.cc#newcode1055
src/ic.cc:1055: // When trying to patch a polymorphic keyed load/store element
stub
The polymorphic stubs that are not load stubs are element load/store stubs.

On 2013/03/01 22:21:04, Jakob wrote:
> Comment seems inaccurate: We end up here when we try to patch anything other
> than a polymorphic load stub.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.cc#n...
src/stub-cache.cc:110: 
On 2013/03/01 22:21:04, Jakob wrote:
> nit: two empty lines

Done.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.cc#n...
src/stub-cache.cc:1390: Handle<JSObject> object,
On 2013/03/01 22:21:04, Jakob wrote:
> Why the reformatting?

Done.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.h
File src/stub-cache.h (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.h#ne...
src/stub-cache.h:628: Handle<JSObject> object,
On 2013/03/01 22:21:04, Jakob wrote:
> Why this reformatting?

Done.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/stub-cache.h#ne...
src/stub-cache.h:641: Handle<JSObject> object,
On 2013/03/01 22:21:04, Jakob wrote:
> Why this reformatting?

Done.

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/type-info.cc
File src/type-info.cc (right):

https://chromiumcodereview.appspot.com/12340112/diff/3001/src/type-info.cc#ne...
src/type-info.cc:506: types->Add(maps.at(i), zone());
On 2013/03/01 22:21:04, Jakob wrote:
> Please compare this with CollectKeyedReceiverTypes() below. In particular,
> you'll want the !CanRetainOtherContext() check here as well.
> Bonus points for more unification of the two methods (use FindAllMaps below?
Or
> factor out the code object walking into yet another helper function that fills
a
> SmallMapList* directly and does the context-retaining check?), but that's
> optional ;-)

Done.

[Toon Verwaest, 2013-03-04 12:46:43]

Ported to ARM and x64. PTAL.

[Jakob Kummerow, 2013-03-04 13:29:35]

lgtm

[Toon Verwaest, 2013-03-04 14:03:42]

Committed manually as r13801 (presubmit successful).