Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(141)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 551093: Implementing inline caches for GenericBinaryOpStub. ... (Closed)

Created:
10 years, 11 months ago by vladislav.kaznacheev
Modified:
9 years, 7 months ago
CC:
v8-dev
Visibility:
Public.

Description

Implementing inline caches for GenericBinaryOpStub. There are 3 fast cases: HeapNumber operands, String operands and Object operands. This CL implements it for ia32 only.

Patch Set 1 #

Total comments: 12

Patch Set 2 : '' #

Total comments: 44
Unified diffs Side-by-side diffs Delta from patch set Stats (+855 lines, -248 lines) Patch
M src/arm/codegen-arm.cc View 1 1 chunk +10 lines, -0 lines 0 comments Download
M src/code-stubs.h View 1 chunk +6 lines, -0 lines 0 comments Download
M src/code-stubs.cc View 3 chunks +18 lines, -2 lines 0 comments Download
M src/debug.cc View 2 chunks +4 lines, -1 line 2 comments Download
M src/ia32/assembler-ia32.h View 1 chunk +2 lines, -0 lines 0 comments Download
M src/ia32/assembler-ia32.cc View 1 chunk +8 lines, -0 lines 0 comments Download
M src/ia32/codegen-ia32.h View 1 8 chunks +85 lines, -9 lines 4 comments Download
M src/ia32/codegen-ia32.cc View 1 18 chunks +637 lines, -223 lines 36 comments Download
M src/ic.h View 1 2 chunks +11 lines, -1 line 0 comments Download
M src/ic.cc View 1 2 chunks +47 lines, -0 lines 2 comments Download
M src/log.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M src/objects.h View 3 chunks +11 lines, -10 lines 0 comments Download
M src/objects.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/objects-inl.h View 1 chunk +2 lines, -2 lines 0 comments Download
M src/spaces.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/x64/codegen-x64.cc View 1 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
vladislav.kaznacheev
10 years, 11 months ago (2010-01-21 12:57:43 UTC) #1
vladislav.kaznacheev
10 years, 11 months ago (2010-01-21 14:16:35 UTC) #2
Erik Corry
I looked at parts of this. I worry that there are too many different stubs ...
10 years, 11 months ago (2010-01-22 09:01:18 UTC) #3
Mads Ager (chromium)
First batch of comments. We should probably measure the code size increase for the extra ...
10 years, 11 months ago (2010-01-22 12:14:26 UTC) #4
vladislav.kaznacheev
10 years, 11 months ago (2010-01-22 14:09:42 UTC) #5 
I have addressed all the comments. Now I will split this CL into two as Mads
suggested: one dealing with registers/smis improvements, another dealing with
ICs.

http://codereview.chromium.org/551093/diff/1/3
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/551093/diff/1/3#newcode744
src/ia32/codegen-ia32.cc:744: enum ArgsLocation {
On 2010/01/22 09:01:18, Erik Corry wrote:
> I would prefer ArgLocation.

Done.

http://codereview.chromium.org/551093/diff/1/3#newcode760
src/ia32/codegen-ia32.cc:760: // Returns operands as floating point numbers on
FPU stack.
On 2010/01/22 09:01:18, Erik Corry wrote:
> No capital letter after a comma.

Done.

http://codereview.chromium.org/551093/diff/1/3#newcode789
src/ia32/codegen-ia32.cc:789: // Accepts operands on stack or in eax,ebx.
On 2010/01/22 09:01:18, Erik Corry wrote:
> Space after comma.

Done.

http://codereview.chromium.org/551093/diff/1/3#newcode799
src/ia32/codegen-ia32.cc:799: case BINARY_OP_STUB_GENERIC: return "Universal";
break;
On 2010/01/22 09:01:18, Erik Corry wrote:
> You don't need a break after a return.

Done.

http://codereview.chromium.org/551093/diff/1/3#newcode1398
src/ia32/codegen-ia32.cc:1398: __ add(answer.reg(), Operand(right->reg()));
There is a smi check several lines above that code, and the deferred code does
not undo the operation, so the operation is not really optimistic.
On 2010/01/22 09:01:18, Erik Corry wrote:
> I think the comment should go back.

http://codereview.chromium.org/551093/diff/1/3#newcode7265
src/ia32/codegen-ia32.cc:7265: ASSERT(kSmiTag == 0);  // adjust zero check if
not the case
On 2010/01/22 09:01:18, Erik Corry wrote:
> adjust -> Adjust
> case -> case.

Done.

http://codereview.chromium.org/551093/diff/3002/3012
File src/debug.cc (right):

http://codereview.chromium.org/551093/diff/3002/3012#newcode129
src/debug.cc:129: && code->kind() != Code::BINARY_OP_IC) ||
On 2010/01/22 12:14:26, Mads Ager wrote:
> I would put the '&&' on the previous line.
> 
> if ((code->is_inline_cache_stub() &&
>      code->kind() != Code::BINARY_OP_IC) ||
>     RelocInfo::Is...(rmode))) {

Done.

http://codereview.chromium.org/551093/diff/3002/3004
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/551093/diff/3002/3004#newcode766
src/ia32/codegen-ia32.cc:766: // Accepts operands on stack or in eax,ebx.
On 2010/01/22 12:14:26, Mads Ager wrote:
> Space after comma.

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode789
src/ia32/codegen-ia32.cc:789: // Accepts operands on stack or in eax,ebx.
On 2010/01/22 12:14:26, Mads Ager wrote:
> Space after comma.

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode799
src/ia32/codegen-ia32.cc:799: case BINARY_OP_STUB_GENERIC: return "Universal";
break;
On 2010/01/22 12:14:26, Mads Ager wrote:
> "Universal" -> "Generic"

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7260
src/ia32/codegen-ia32.cc:7260: case Token::ADD: __ sub(eax, Operand(ebx));
break;
On 2010/01/22 12:14:26, Mads Ager wrote:
> Add comment that says that this reverts the optimistic operation.

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7277
src/ia32/codegen-ia32.cc:7277: __ AllocateHeapNumber(edx, ecx, no_reg,
On 2010/01/22 12:14:26, Mads Ager wrote:
> Reindent argument list:
> 
> __ AllocateHeapNumber(
>     edx,
>     ecx,
>     no_reg,
>     HasArgumentsInRegister() ? ...);
> 

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7314
src/ia32/codegen-ia32.cc:7314: // These operations always succeed on a pair of
smis.
We never get to this place when executing, but we can get here while generating.
Inserted "Do nothing here" at the start of the comment to make it (hopefully)
clearer.

Fixed indentation.
On 2010/01/22 12:14:26, Mads Ager wrote:
> That means that we should never get to this place, right?  In that case,
please
> put in a call to UNREACHABLE() here.
> 
> Also, indentation seems off by one here.  Indented by three instead of two?

http://codereview.chromium.org/551093/diff/3002/3004#newcode7319
src/ia32/codegen-ia32.cc:7319: // These go directly to runtime
See previous reply.
On 2010/01/22 12:14:26, Mads Ager wrote:
> Similarly here: UNREACHABLE()?
> 
> Three-space indented instead of two?

http://codereview.chromium.org/551093/diff/3002/3004#newcode7339
src/ia32/codegen-ia32.cc:7339: default: UNREACHABLE(); break;
On 2010/01/22 12:14:26, Mads Ager wrote:
> Indentation is off for the cases in this switch.  The end '}' should align
with
> the 'default:'.  It seems the case is four-space indented instead of two-space
> indented?

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7419
src/ia32/codegen-ia32.cc:7419: // For MOD we go directly to runtime in the
non-smi case.
Similarly, this line is executed on code generation, but no code needs to be
generated. Removing this case will case the default branch call UNREACHABLE().
On 2010/01/22 12:14:26, Mads Ager wrote:
> UNREACHABLE()?

http://codereview.chromium.org/551093/diff/3002/3004#newcode7506
src/ia32/codegen-ia32.cc:7506: // are already in edx,eax
On 2010/01/22 12:14:26, Mads Ager wrote:
> Space after comma.

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7516
src/ia32/codegen-ia32.cc:7516: // First argument is a a string, test second.
On 2010/01/22 12:14:26, Mads Ager wrote:
> a a -> a

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7570
src/ia32/codegen-ia32.cc:7570: break;
On 2010/01/22 12:14:26, Mads Ager wrote:
> indentation.

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7599
src/ia32/codegen-ia32.cc:7599: // Generate the unreachable reference to the
original stub so that it can be
On 2010/01/22 12:14:26, Mads Ager wrote:
> the -> an

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7625
src/ia32/codegen-ia32.cc:7625: (fast_case_ == BINARY_OP_STUB_UNINITIALIZED)) {
On 2010/01/22 12:14:26, Mads Ager wrote:
> Indent one more space.
> 
> Could you add a comment explaining the ShouldGenerateSmiCode() part of this
> condition?

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7654
src/ia32/codegen-ia32.cc:7654: __ push(eax);
On 2010/01/22 12:14:26, Mads Ager wrote:
> Add comment explaining what eax is here to make this readable as a standalone
> method.

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7656
src/ia32/codegen-ia32.cc:7656: //  __ push(eax);
On 2010/01/22 12:14:26, Mads Ager wrote:
> Code in comment.

Done.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7680
src/ia32/codegen-ia32.cc:7680: __ pop(eax);
There is always a tail call following this so we never break anything. Added a
comment clarifying this.

On 2010/01/22 12:14:26, Mads Ager wrote:
> Popping the arguments instead of loading them seems dangerous to me.  After
> popping, there will be two missing arguments on the stack under the return
> address - does the return code handle that correctly or will we pop again on
> return?
> 
> Or do we always tail-call something at some later point so that this is safe? 
> We should see if we can make this clearer and at least add comments explaining
> why this is ok.

http://codereview.chromium.org/551093/diff/3002/3004#newcode7762
src/ia32/codegen-ia32.cc:7762: Label* alloc_failure) {
On 2010/01/22 12:14:26, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/551093/diff/3002/3005
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/551093/diff/3002/3005#newcode704
src/ia32/codegen-ia32.h:704: // Minor key encoding in 16 bits CCCFRASOOOO0OOOMM.
On 2010/01/22 12:14:26, Mads Ager wrote:
> I think there is a zero '0' instead of the letter 'O' here? 
Made sure those are letter 'O's

http://codereview.chromium.org/551093/diff/3002/3005#newcode751
src/ia32/codegen-ia32.h:751: return HasSmiCodeInStub() && (fast_case_ <
BINARY_OP_STUB_FAST_FP);
This is taken care of in GetFastCase method. It makes sure that if
NO_SMI_CODE_IN_STUB is set the FAST_FP stub is never generated (GENERIC is
generated insted).
On 2010/01/22 12:14:26, Mads Ager wrote:
> This is not really related to this method itself, but can we get in a
situation
> where HasSmiCodeInStub() returns true and fast_case_ ==
BINARY_OP_STUB_FAST_FP? 
> We should make sure that we cannot get in that situation to make sure that we
do
> not get two different stubs with the same code.
> 
> I guess we only want the Smi code for the generic stub?  Test explicitly for
> fast_case_ == BINARY_OP_STUB_GENERIC?

http://codereview.chromium.org/551093/diff/3002/3008
File src/ic.cc (right):

http://codereview.chromium.org/551093/diff/3002/3008#newcode1460
src/ic.cc:1460: Object * BinaryOp_Patch(Arguments args) {
On 2010/01/22 12:14:26, Mads Ager wrote:
> Remove space before '*'.

Done.

Powered by Google App Engine
This is Rietveld 408576698