Issue 6366028: X64 Crankshaft: Add TypeRecordingBinaryStub to X64

Issue 6366028: X64 Crankshaft: Add TypeRecordingBinaryStub to X64 (Closed)

Created:
9 years, 11 months ago by William Hesse

Modified:
9 years, 7 months ago

Reviewers:
Søren Thygesen Gjesse

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

X64 Crankshaft: Add TypeRecordingBinaryStub to X64 Committed: http://code.google.com/p/v8/source/detail?r=6622

Patch Set 6 : '' #

Created: 9 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+438 lines, -90 lines)			Patch
M	src/ic.cc	View	1 2 3 4	2 chunks	+5 lines, -1 line	0 comments	Download
M	src/type-info.h	View	1 2 3 4	1 chunk	+3 lines, -3 lines	0 comments	Download
M	src/x64/code-stubs-x64.h	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	src/x64/code-stubs-x64.cc	View	1 2 3 4 5	12 chunks	+386 lines, -64 lines	0 comments	Download
M	src/x64/full-codegen-x64.cc	View	1 2 3 4 5	4 chunks	+16 lines, -21 lines	0 comments	Download
M	src/x64/ic-x64.cc	View	1 2 3 4	1 chunk	+2 lines, -1 line	0 comments	Download
M	src/x64/macro-assembler-x64.h	View	1 2 3 4	3 chunks	+21 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Søren Thygesen Gjesse

LGTM http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1151 src/x64/code-stubs-x64.cc:1151: __ SmiMod(rax, left, right, &restore_MOD_registers); Do you need ...

9 years, 10 months ago (2011-02-03 08:00:43 UTC) #2

William Hesse

9 years, 10 months ago (2011-02-04 14:37:25 UTC) #3

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1151: __ SmiMod(rax, left, right,
&restore_MOD_registers);
No.  Eliminated.

On 2011/02/03 08:00:43, Søren Gjesse wrote:
> Do you need the label "restore_MOD_registers"? It is bound the same place as
> "use_fp_on_smis" where there is a check for both DIV and MOD.

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1205: if (generate_inline_heapnumber_results) {
On 2011/02/03 08:00:43, Søren Gjesse wrote:
> Can't you take mode_ into account here and reuse an overwritable heap number
if
> any?

We only reach here if the inputs were both Smis.  Otherwise, we skip to the end
of the Smi code.

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1226: if (op_ == Token::BIT_OR) {
On 2011/02/03 08:00:43, Søren Gjesse wrote:
> rax -> right?

Done.

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1300: // Not using AllocateHeapNumber macro in order
to reuse
On 2011/02/03 08:00:43, Søren Gjesse wrote:
> How about creating an AllocateHeapNumber in macro assembler which takes an
> additional register with the heap number map?

It is only used this one place, and it is really just two lines plus a debug
check.  So I don't think it is worth it.

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1347: Condition is_smi;
On 2011/02/03 08:00:43, Søren Gjesse wrote:
> Please remove this debugging code.

Done.

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1350: is_smi = masm->CheckSmi(lhs);
On 2011/02/03 08:00:43, Søren Gjesse wrote:
> The code pattern for checking for a string is repeated three times. How about
> JumpIfNotString in macro assembler?

In one of the three places, the smi check jumps to a different place than the
string check.  We already have a macro for the Smi check, so the remaining
string check becomes 2 lines.  So I used the macro for the smi check.

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1451: __ bind(&call_runtime);
There should be no ALLOW_HEAPNUMBER_RESULTS case on X64, except when generating
the generic stub.
Removed.

On 2011/02/03 08:00:43, Søren Gjesse wrote:
> Should there be a separate exit for failed heap allocation in the
> ALLOW_HEAPNUMBER_RESULTS case which does not perform a type transition, but
just
> calls runtime?

http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1465: Label call_runtime, type_transition;
On 2011/02/03 08:00:43, Søren Gjesse wrote:
> Maybe rename the labels
> 
>   call_runtime -> gc_required
>   type_transition -> not_number
> 
> to make it more clear why the exits are called.

Done.

http://codereview.chromium.org/6366028/diff/6010/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/6366028/diff/6010/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:3209: __ j(is_smi, &done);
On 2011/02/03 08:00:43, Søren Gjesse wrote:
> Remove empty line?

I like an empty line before a new section of generated code, that does not
expect fall-through in most cases.

Expand Messages | Collapse Messages