Implementing inline caches for binary operations (ia32)....

Implementing inline caches for binary operations (ia32).

This is a subset of a CL reviewed earlier(http://codereview.chromium.org/551093).
The register usage optimisation part has been reviewed and submitted separately.
Two fast cases supported: HeapNumber operands and String operands for ADD.



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

[Vladislav Kaznacheev, 2010-02-02 23:25:39]

Kevin asked me if it is possible to do the same with less complexity and more
similarity to other types of ICs. I rewrote most of the CL. It is still very
different from existing ICS, but I thinks it is now much more transparent.

[Kevin Millikin (Chromium), 2010-02-08 12:06:56]

I think it's getting clearer.  My big concern is still that it's too different
from the other inline caching mechanism.

I think my problem is this: we have two kinds of stubs (1) inline cache stubs we
use for performance, which inherit from class IC and are passed based on runtime
type information and (2) shared code stubs we use to reduce code size, which
inherit from class CodeStub and are not patched.

This change is complicated, because it combines the two.  Perhaps it would be
better to introduce a new class of BinaryOpIC inline chache stubs instead of
trying to patch the existing GenericBinaryOpStub code stubs.  I think that would
be conceptually clearer.

http://codereview.chromium.org/553117/diff/2001/3014
File src/code-stubs.h (right):

http://codereview.chromium.org/553117/diff/2001/3014#newcode141
src/code-stubs.h:141: virtual int GetCodeKind();
This should return Code::Kind.

http://codereview.chromium.org/553117/diff/2001/3014#newcode144
src/code-stubs.h:144: virtual int GetICState();
This should return InlineCacheState.

http://codereview.chromium.org/553117/diff/2001/3008
File src/debug.cc (right):

http://codereview.chromium.org/553117/diff/2001/3008#newcode1367
src/debug.cc:1367: UNREACHABLE();
This whole block of code should be cleaned up to use a switch over code->kind().

http://codereview.chromium.org/553117/diff/2001/3003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/553117/diff/2001/3003#newcode31
src/ia32/codegen-ia32.h:31: #include "ic-inl.h"
Is there a reason this can't be #include "ic.h"?

[Vladislav Kaznacheev, 2010-02-09 01:38:08]

Kevin,

I am not sure I understand your idea. 

One way I can think of would be like this:
1. CodeGenerator should generate calls to BinaryOpIC_Miss stub. 
2. The stub should analyze the types of operands and do the patching to one of
the GenericBinaryOpStub variants.
3. Before returning the stub should calculate the operation result. The only
reasonable way to do that would be to call the existing ("default")
GenericBinaryOpStub, because the runtime implementation simply does not do all
that is necessary. 

My current implementation is basically doing all this, only one of the versions
of GenericBinaryOpStub serves as a _Miss stub. I believe that separating the two
will lead to longer and more complex code (_Miss stubs will need to be generated
for all possible combinations of GenericBinaryOpStub parameters and I will need
to clone minor key encoding methods from GenericBinaryOpStub for each platform).

Also in my implementation if a stub always gets Smi arguments (or HeapNumbers
when NO_SMI_CODE_IN_STUB is set) then patching does not happen at all, so we
save time on patching and clearing ICs.

Of course I might be completely missing your idea.

http://codereview.chromium.org/553117/diff/2001/3014
File src/code-stubs.h (right):

http://codereview.chromium.org/553117/diff/2001/3014#newcode141
src/code-stubs.h:141: virtual int GetCodeKind();
True. I was unable to do that because Code::Kind is defined in objects.h which
includes this file (code-stubs.h) to use CodeStub.Major. I can see two ways to
break this circular dependency between Code and CodeStub:
1. Move Code::Kind definition out of Code (to globals.h?). This will require
changing Code::STUB (and the like) to STUB all over the codebase. Also this will
require renaming TypeCode.BUILTIN to resolve a name clash in the global scope.

2. Move CodeStub::Major definition out of code-stubs.h. This is probably less
work, but I am not sure.

Which of the two ways above would you recommend?

Though I would rather not do anything before we agree on the general approach to
ICs (maybe it will all be unnecessary). Still, 

On 2010/02/08 12:06:56, Kevin Millikin wrote:
> This should return Code::Kind.

http://codereview.chromium.org/553117/diff/2001/3014#newcode144
src/code-stubs.h:144: virtual int GetICState();
Done. Including globals.h helped.
On 2010/02/08 12:06:56, Kevin Millikin wrote:
> This should return InlineCacheState.

http://codereview.chromium.org/553117/diff/2001/3008
File src/debug.cc (right):

http://codereview.chromium.org/553117/diff/2001/3008#newcode1367
src/debug.cc:1367: UNREACHABLE();
On 2010/02/08 12:06:56, Kevin Millikin wrote:
> This whole block of code should be cleaned up to use a switch over
code->kind().

Done.

http://codereview.chromium.org/553117/diff/2001/3003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/553117/diff/2001/3003#newcode31
src/ia32/codegen-ia32.h:31: #include "ic-inl.h"
There was (errors like "inline function 'static v8::internal::Code*
v8::internal::IC::GetTargetAtAddress(v8::internal::byte*)' used but never
defined" for every file that includes codegen-ia32.h). I moved two inline
functions
 from ic.h to ic-inl.h and now it's all right. 
On 2010/02/08 12:06:56, Kevin Millikin wrote:
> Is there a reason this can't be #include "ic.h"?

[Kevin Millikin (Chromium), 2010-02-18 10:31:34]

It looks much cleaner now.

There is a GC issue in BinarOp_Patch that needs to be fixed, but the rest looks
like a good start.

http://codereview.chromium.org/553117/diff/13001/14015
File src/code-stubs.h (right):

http://codereview.chromium.org/553117/diff/13001/14015#newcode144
src/code-stubs.h:144: virtual int GetCodeKind();
As we discussed, after this change lands we should try to see if the binary op
stubs can be made into ICs (inheriting from class IC).

For example, it's strange that a code stub reports a kind XXX_IC instead of
STUB.

http://codereview.chromium.org/553117/diff/13001/14002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/553117/diff/13001/14002#newcode7909
src/ia32/codegen-ia32.cc:7909: // Generate an unreachable reference to the
UNKNOWN stub so that it can be
UNKNOWN ==> DEFAULT

http://codereview.chromium.org/553117/diff/13001/14002#newcode7911
src/ia32/codegen-ia32.cc:7911: if (runtime_operands_type_ !=
BinaryOpIC::DEFAULT) {
As we discussed, we should (a) evaluate whether we want to clear these or not
and (b) if so find a more elegant way to encode the default stub.

http://codereview.chromium.org/553117/diff/13001/14002#newcode8005
src/ia32/codegen-ia32.cc:8005: GenerateRegisterArgsPush(masm);
I find it a little confusing that GenerateRegisterArgsPush doesn't generate
anything if !HasArgsInRegisters() and GenerateLoadArguments doesn't generate
anything if HasArgsInRegisters.

I'd like it better if it was assumed (ASSERTed) that HasArgsInRegisters for
GenerateRegisterArgsPush and !HasArgsInRegisters for GenerateLoadArguments. 
Then the code here becomes:

// Keep a copy of operands on the stack and make sure they are also in edx,eax.
if (HasArgsInRegister()) {
  GenerateRegisterArgsPush(masm);
} else {
  GenerateLoadArguments(masm);
}

And all the other call sites changed similarly.

http://codereview.chromium.org/553117/diff/13001/14004
File src/ic.cc (right):

http://codereview.chromium.org/553117/diff/13001/14004#newcode1409
src/ic.cc:1409: Code * uninit_stub =
Code::GetCodeFromTargetAddress(rinfo->target_address());
Should be no space between Code and *.

http://codereview.chromium.org/553117/diff/13001/14004#newcode1451
src/ic.cc:1451: //  if (left->IsString() || right->IsString()) {
Don't commit commented-out code.

http://codereview.chromium.org/553117/diff/13001/14004#newcode1466
src/ic.cc:1466: NoHandleAllocation na;
This function looks risky for GC.  The call to GetBinaryOpStub will call
GetCode, which may have to compile the stub and allocate, which may trigger a
GC.

If that happens, the raw Object pointers will not be safe to use (specifically,
result).

Solution is to put left, right, result in handles:

Handle<Object> left = args.at<Object>(0);
Handle<Object> right = args.at<Object>(1);

Then put a HandleScope in this function around the call to GetBinaryOpStub and
the code that uses its result.  Remove the HandleScope from GetBinaryOpStub and
make it return Handle<Code>.

There's an example of the style in this file in CallIC_Miss.

http://codereview.chromium.org/553117/diff/13001/14006
File src/ic.h (right):

http://codereview.chromium.org/553117/diff/13001/14006#newcode450
src/ic.h:450: DEFAULT,
The distinction between DEFAULT and GENERIC is not obvious.  Maybe a comment
will help.

http://codereview.chromium.org/553117/diff/13001/14006#newcode458
src/ic.h:458: void patch(Code* code);
Is this function needed?

[Vladislav Kaznacheev, 2010-02-25 16:19:43]

I have addressed most of the comments. Still not sure what to do with
BinaryOpIC::patch. Also uploaded a rebased patch.

http://codereview.chromium.org/553117/diff/13001/14002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/553117/diff/13001/14002#newcode7909
src/ia32/codegen-ia32.cc:7909: // Generate an unreachable reference to the
UNKNOWN stub so that it can be
On 2010/02/18 10:31:34, Kevin Millikin wrote:
> UNKNOWN ==> DEFAULT

Done.

http://codereview.chromium.org/553117/diff/13001/14002#newcode8005
src/ia32/codegen-ia32.cc:8005: GenerateRegisterArgsPush(masm);
On 2010/02/18 10:31:34, Kevin Millikin wrote:
> I find it a little confusing that GenerateRegisterArgsPush doesn't generate
> anything if !HasArgsInRegisters() and GenerateLoadArguments doesn't generate
> anything if HasArgsInRegisters.
> 
> I'd like it better if it was assumed (ASSERTed) that HasArgsInRegisters for
> GenerateRegisterArgsPush and !HasArgsInRegisters for GenerateLoadArguments. 
> Then the code here becomes:
> 
> // Keep a copy of operands on the stack and make sure they are also in
edx,eax.
> if (HasArgsInRegister()) {
>   GenerateRegisterArgsPush(masm);
> } else {
>   GenerateLoadArguments(masm);
> }
> 
> And all the other call sites changed similarly.

Done.

http://codereview.chromium.org/553117/diff/13001/14004
File src/ic.cc (right):

http://codereview.chromium.org/553117/diff/13001/14004#newcode1409
src/ic.cc:1409: Code * uninit_stub =
Code::GetCodeFromTargetAddress(rinfo->target_address());
On 2010/02/18 10:31:34, Kevin Millikin wrote:
> Should be no space between Code and *.

Done.

http://codereview.chromium.org/553117/diff/13001/14004#newcode1451
src/ic.cc:1451: //  if (left->IsString() || right->IsString()) {
On 2010/02/18 10:31:34, Kevin Millikin wrote:
> Don't commit commented-out code.

Done.

http://codereview.chromium.org/553117/diff/13001/14004#newcode1466
src/ic.cc:1466: NoHandleAllocation na;
On 2010/02/18 10:31:34, Kevin Millikin wrote:
> This function looks risky for GC.  The call to GetBinaryOpStub will call
> GetCode, which may have to compile the stub and allocate, which may trigger a
> GC.
> 
> If that happens, the raw Object pointers will not be safe to use
(specifically,
> result).
> 
> Solution is to put left, right, result in handles:
> 
> Handle<Object> left = args.at<Object>(0);
> Handle<Object> right = args.at<Object>(1);
> 
> Then put a HandleScope in this function around the call to GetBinaryOpStub and
> the code that uses its result.  Remove the HandleScope from GetBinaryOpStub
and
> make it return Handle<Code>.
> 
> There's an example of the style in this file in CallIC_Miss.

Done.

http://codereview.chromium.org/553117/diff/13001/14006
File src/ic.h (right):

http://codereview.chromium.org/553117/diff/13001/14006#newcode450
src/ic.h:450: DEFAULT,
On 2010/02/18 10:31:34, Kevin Millikin wrote:
> The distinction between DEFAULT and GENERIC is not obvious.  Maybe a comment
> will help.

Added a comment.

http://codereview.chromium.org/553117/diff/13001/14006#newcode458
src/ic.h:458: void patch(Code* code);
On 2010/02/18 10:31:34, Kevin Millikin wrote:
> Is this function needed?

The only reason for its existence is that IC::set_target is protected. I could
either merge this function into BinaryOpIC constructor or make IC::set_target
public but I am not sure I like any of the two options.

[Kevin Millikin (Chromium), 2010-03-01 15:57:08]

LGTM.

http://codereview.chromium.org/553117/diff/19001/18005
File src/ic.cc (right):

http://codereview.chromium.org/553117/diff/19001/18005#newcode1494
src/ic.cc:1494: HandleScope scope;
HandleScope could be moved down to before GetBinaryOpStub because you don't need
a handle scope to create handle copies of the args (they're in the stack, so
seen and updated on GC):

{ HandleScope scope;
  BinaryOpIC::TypeInfo type_info = ...
  ...
  if (!code.is_null()) {
    ...
  }
}

return *result;