Record AST ids in relocation info at spots where we collect dynamic type feedback.

Record AST ids in relocation info at spots where we collect dynamic type feedback.

Committed: http://code.google.com/p/v8/source/detail?r=7632

[William Hesse, 2011-04-14 15:07:05]

A gigantic code review for you.
I have not yet ensured that ARM gets the correct optimizations, by checking its
performance, just that it passes all tests.

[Vitaly Repeshko, 2011-04-15 01:24:43]

LGTM for the non-arm parts if we don't get duplicate ids in the type oracle.


Thanks,
Vitaly

http://codereview.chromium.org/6793016/diff/18001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:2698: 
nit: Remove blank line.

http://codereview.chromium.org/6793016/diff/18001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:2729: RelocInfo reloc_info_with_ast_id(pc_, rmode,
ast_id_for_reloc_info_);
Not using data looks like a hack. There must be a better way.

http://codereview.chromium.org/6793016/diff/18001/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:171: mov(ip, Operand(target, rmode), LeaveCC,
cond);
What if USE_BLX is on?

http://codereview.chromium.org/6793016/diff/18001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:219: RelocInfo::Mode rmode,
nit: Fix indentation.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode98
src/assembler.cc:98: //
It'd be nice to describe the tag structure, i.e. the two least significant bits
are "tag", the next four bits are "extra tag", and finally the two most
significant bits are "top tag".

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode132
src/assembler.cc:132: // data-jump + position :    00 1110 11,
We need a similar "Written by" comment here.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode155
src/assembler.cc:155: const int kPCJumpTag = (1 << kExtraTagBits) - 1;
Let's rename this to kPCJumpExtraTag.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode169
src/assembler.cc:169: const int kDataJumpTag = kPCJumpTag - 1;
kDataJumpExtraTag

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode225
src/assembler.cc:225: {
nit: Put '{' on the previous line.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode249
src/assembler.cc:249: ASSERT(RelocInfo::NUMBER_OF_MODES -
RelocInfo::LAST_CODE_ENUM <=
LAST_COMPACT_ENUM?

http://codereview.chromium.org/6793016/diff/18001/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/6793016/diff/18001/src/assembler.h#newcode235
src/assembler.h:235: LAST_GCED_ENUM = EMBEDDED_OBJECT,
I think GLOBAL_PROPERTY_CELL should stay the last gc-ed.

http://codereview.chromium.org/6793016/diff/18001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/6793016/diff/18001/src/ia32/assembler-ia32.h#n...
src/ia32/assembler-ia32.h:853: unsigned ast_is = kNoASTId);
ast_is -> ast_id

http://codereview.chromium.org/6793016/diff/18001/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/type-info.cc#newcode344
src/type-info.cc:344: MaybeObject* maybe_result =
dictionary_->AtNumberPut(ast_id, target);
It used to work fine with an extra assert that we have no duplicates in the map.
Could you please try to restore the assert?

http://codereview.chromium.org/6793016/diff/18001/src/type-info.cc#newcode410
src/type-info.cc:410: List<int>* code_positions,
nit: Fix indentation.

http://codereview.chromium.org/6793016/diff/18001/src/type-info.cc#newcode417
src/type-info.cc:417: if (RelocInfo::IsCodeTarget(mode)) {
This can be turned into an assert.

[William Hesse, 2011-04-15 11:51:27]

http://codereview.chromium.org/6793016/diff/18001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:2729: RelocInfo reloc_info_with_ast_id(pc_, rmode,
ast_id_for_reloc_info_);
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> Not using data looks like a hack. There must be a better way.

We discussed it here, and there is no better way.  If we add a parameter, it has
to be threaded through many levels of functions and objects, including a field
on Operand.  This is a extra bit of data that is not used in most cases.

The data field has already been hijacked to store the target of a relocation,
until the constant pool is written, so it is not available.

http://codereview.chromium.org/6793016/diff/18001/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:171: mov(ip, Operand(target, rmode), LeaveCC,
cond);
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> What if USE_BLX is on?

The ast_id is carried through Assembler::ast_id_for_reloc_info_, and an rmode of
CODE_TARGET_WITH_ID.  The ast_id argument below is a mistake, and would not even
compile.  Removed.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode98
src/assembler.cc:98: //
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> It'd be nice to describe the tag structure, i.e. the two least significant
bits
> are "tag", the next four bits are "extra tag", and finally the two most
> significant bits are "top tag".

I have a rewrite of this, that I forgot to copy in.  Here it is.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode155
src/assembler.cc:155: const int kPCJumpTag = (1 << kExtraTagBits) - 1;
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> Let's rename this to kPCJumpExtraTag.

Done.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode169
src/assembler.cc:169: const int kDataJumpTag = kPCJumpTag - 1;
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> kDataJumpExtraTag

Done.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode225
src/assembler.cc:225: {
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> nit: Put '{' on the previous line.

Done.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.cc#newcode249
src/assembler.cc:249: ASSERT(RelocInfo::NUMBER_OF_MODES -
RelocInfo::LAST_CODE_ENUM <=
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> LAST_COMPACT_ENUM?

Done.

http://codereview.chromium.org/6793016/diff/18001/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/6793016/diff/18001/src/assembler.h#newcode235
src/assembler.h:235: LAST_GCED_ENUM = EMBEDDED_OBJECT,
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> I think GLOBAL_PROPERTY_CELL should stay the last gc-ed.

Done.

http://codereview.chromium.org/6793016/diff/18001/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/6793016/diff/18001/src/type-info.cc#newcode344
src/type-info.cc:344: MaybeObject* maybe_result =
dictionary_->AtNumberPut(ast_id, target);
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> It used to work fine with an extra assert that we have no duplicates in the
map.
> Could you please try to restore the assert?

Done.

http://codereview.chromium.org/6793016/diff/18001/src/type-info.cc#newcode417
src/type-info.cc:417: if (RelocInfo::IsCodeTarget(mode)) {
On 2011/04/15 01:24:43, Vitaly Repeshko wrote:
> This can be turned into an assert.

Done.

[fschneider, 2011-04-15 12:27:14]

LGTM.

http://codereview.chromium.org/6793016/diff/27001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/6793016/diff/27001/src/arm/assembler-arm.h#new...
src/arm/assembler-arm.h:1333: 
Remove extra new line.

http://codereview.chromium.org/6793016/diff/27001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/6793016/diff/27001/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:3983: EmitCallIC(ic, RelocInfo::CODE_TARGET_WITH_ID,
expr->id());
I think it would work, but maybe we should for consistency use the same id that
we use for the PrepareBailout here: expr->AssignmentId()

http://codereview.chromium.org/6793016/diff/27001/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:4000: EmitCallIC(ic, RelocInfo::CODE_TARGET_WITH_ID,
expr->id());
expr->AssignmentId() also here (and on the other platforms)

http://codereview.chromium.org/6793016/diff/27001/src/full-codegen.h
File src/full-codegen.h (right):

http://codereview.chromium.org/6793016/diff/27001/src/full-codegen.h#newcode516
src/full-codegen.h:516: void EmitCallIC(Handle<Code> ic,
I'd prefer a mandatory argument since forgetting to record an id will result in
hard-to-find performance degradations.

http://codereview.chromium.org/6793016/diff/27001/src/full-codegen.h#newcode523
src/full-codegen.h:523: void EmitCallIC(Handle<Code> ic,
Same here.