Improve our type feedback by recogizining never-executed IC calls for binary ...

Improve our type feedback by recogizining never-executed IC calls for binary operations.

In the case of inlined smi code in non-optimzied code we could not 
distinguish between the smi-only case and the case that the operation was
never executed.

With this change the first execution of a binary operation always jumps
to the stub which in turn patches the smi-check into the correct
conditional branch, so that we benefit from inlined smi code after the
first invocation.

A nop instruction after the call to the BinaryOpIC indicates that no
smi code was inlined. A "test eax" instruction says that there was smi
code inlined and encodes the delta to the patch site and the condition
code of the branch at the patch site to restore the original jump.


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

[fschneider, 2010-12-09 14:35:50]

Please have a look.

[fschneider, 2010-12-09 14:43:54]

Adding Vitaly.

This may also be useful to detect more call sites that are never executed before
optimizing. I plan to do the same for the CompareIC as well.

[Vitaly Repeshko, 2010-12-09 15:59:01]

LGTM!

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1557: masm_->bind(&patch_site_);
In debug mode in ~JumpPatchSite we could assert that patch info is emitted if
patch_site is bound.

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1563: __ test(eax, Immediate(delta_to_patch_site
<< 16 | cc_));
For smi checks the condition is always not_zero. We could save 3 bytes by using
short form of test here. Of course, this makes JumpPatchSite less generic. On
the other hand, we could specialize it even more for smi checks, e.g.:
SmiCheckPatchSite smi_check(masm_, &call_stub);
smi_check.EmitCheck(eax);
...
CallStub(...);
smi_check.EmitPatchInfo();

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1597: __ CallStub(&stub);
Consider adding a function like EmitCallIC that takes both a stub to call and an
optional patch site. Patch site should be used if it is bound, otherwise nop is
emitted.

[William Hesse, 2010-12-09 16:27:58]

LGTM.

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

http://codereview.chromium.org/5714001/diff/9001/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:576: static const byte kJmpShortOpcode = 0xeb;
I think we tend to use capital letters in hexadecimal constants in the
assembler.

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1597: __ CallStub(&stub);
On 2010/12/09 15:59:01, Vitaly wrote:
> Consider adding a function like EmitCallIC that takes both a stub to call and
an
> optional patch site. Patch site should be used if it is bound, otherwise nop
is
> emitted.

I'm not sure this is a good idea, since the existing call to CallStub is at a
nice low level, and all the other JumpSitePatch relevant stuff is right here in
the same code.

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3767: __ jmp(&done);
Too bad that you have to put an extra jump here, to make it work.

[Kevin Millikin (Chromium), 2010-12-10 06:41:41]

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1597: __ CallStub(&stub);
I like Vitaly's idea because it's safer.  I think the jumping/branching and the
calling is something you want to see in this code generator code.  The required
__nop() or EmitPatchInfo is bookkeeping and doesn't need to be seen here.

 Taking it a step further, you could make it read more like a jump by making the
interface:

JumpPatchSite patch_site(masm_);
patch_site.EmitJump(not_zero, &call_stub);
...

EmitCallStub(&stub, &patch_site);

[fschneider, 2010-12-10 13:19:24]

Thanks for the comments.

I uploaded a new version which does the same for CompareIC.

I also needed to insert nop when calling the stub from the lithium code
generator.

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

http://codereview.chromium.org/5714001/diff/9001/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:576: static const byte kJmpShortOpcode = 0xeb;
On 2010/12/09 16:27:58, William Hesse wrote:
> I think we tend to use capital letters in hexadecimal constants in the
> assembler.

Done.

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1557: masm_->bind(&patch_site_);
On 2010/12/09 15:59:01, Vitaly wrote:
> In debug mode in ~JumpPatchSite we could assert that patch info is emitted if
> patch_site is bound.

Done.

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1563: __ test(eax, Immediate(delta_to_patch_site
<< 16 | cc_));
On 2010/12/09 15:59:01, Vitaly wrote:
> For smi checks the condition is always not_zero. We could save 3 bytes by
using
> short form of test here. Of course, this makes JumpPatchSite less generic. On
> the other hand, we could specialize it even more for smi checks, e.g.:
> SmiCheckPatchSite smi_check(masm_, &call_stub);
> smi_check.EmitCheck(eax);
> ...
> CallStub(...);
> smi_check.EmitPatchInfo();

Done.

http://codereview.chromium.org/5714001/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1597: __ CallStub(&stub);
On 2010/12/10 06:41:41, Kevin Millikin wrote:
> I like Vitaly's idea because it's safer.  I think the jumping/branching and
the
> calling is something you want to see in this code generator code.  The
required
> __nop() or EmitPatchInfo is bookkeeping and doesn't need to be seen here.
> 
>  Taking it a step further, you could make it read more like a jump by making
the
> interface:
> 
> JumpPatchSite patch_site(masm_);
> patch_site.EmitJump(not_zero, &call_stub);
> ...
> 
> EmitCallStub(&stub, &patch_site);

Done.

[Vitaly Repeshko, 2010-12-10 13:38:56]

Still LGTM.

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:47: JumpPatchSite(MacroAssembler* masm) :
masm_(masm), info_emitted_(false) { }
Missing "explicit".

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:66: bool is_bound() { return
patch_site_.is_bound(); }
Can it be made const?

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:71: bool info_emitted_;
Should probably be guarded by #ifdef DEBUG.

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:1891: __ CallStub(&stub);
How about making patch site an optional parameter to EmitCallIC so that you can
use EmitCallIC(&stub, NULL) in cases like this to get the nop generated?

http://codereview.chromium.org/5714001/diff/30001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/30001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1093: __ nop();  // Signals no inlined smi
code.
You can make CallCode emit the nop, if the code is a binary IC stub.

http://codereview.chromium.org/5714001/diff/30001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/5714001/diff/30001/src/ic.cc#newcode2135
src/ic.cc:2135: CompareIC::State CompareIC::TargetState(State state,
Handle<Object> x, Handle<Object> y) {
Line too long.

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

http://codereview.chromium.org/5714001/diff/30001/src/type-info.cc#newcode144
src/type-info.cc:144: case CompareIC::UNINITIALIZED:
Return unknown?

http://codereview.chromium.org/5714001/diff/30001/src/type-info.cc#newcode228
src/type-info.cc:228: case CompareIC::UNINITIALIZED:
Same here.

[fschneider, 2010-12-10 14:32:17]

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:47: JumpPatchSite(MacroAssembler* masm) :
masm_(masm), info_emitted_(false) { }
On 2010/12/10 13:38:56, Vitaly wrote:
> Missing "explicit".

Done.

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:66: bool is_bound() { return
patch_site_.is_bound(); }
On 2010/12/10 13:38:56, Vitaly wrote:
> Can it be made const?

Done.

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:71: bool info_emitted_;
On 2010/12/10 13:38:56, Vitaly wrote:
> Should probably be guarded by #ifdef DEBUG.

Done.

http://codereview.chromium.org/5714001/diff/30001/src/ia32/full-codegen-ia32....
src/ia32/full-codegen-ia32.cc:1891: __ CallStub(&stub);
On 2010/12/10 13:38:56, Vitaly wrote:
> How about making patch site an optional parameter to EmitCallIC so that you
can
> use EmitCallIC(&stub, NULL) in cases like this to get the nop generated?

Done.

http://codereview.chromium.org/5714001/diff/30001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5714001/diff/30001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1093: __ nop();  // Signals no inlined smi
code.
On 2010/12/10 13:38:56, Vitaly wrote:
> You can make CallCode emit the nop, if the code is a binary IC stub.

Done. (also for the CompareIC)

http://codereview.chromium.org/5714001/diff/30001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/5714001/diff/30001/src/ic.cc#newcode2135
src/ic.cc:2135: CompareIC::State CompareIC::TargetState(State state,
Handle<Object> x, Handle<Object> y) {
On 2010/12/10 13:38:56, Vitaly wrote:
> Line too long.

Done.

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

http://codereview.chromium.org/5714001/diff/30001/src/type-info.cc#newcode144
src/type-info.cc:144: case CompareIC::UNINITIALIZED:
On 2010/12/10 13:38:56, Vitaly wrote:
> Return unknown?

Done.

http://codereview.chromium.org/5714001/diff/30001/src/type-info.cc#newcode228
src/type-info.cc:228: case CompareIC::UNINITIALIZED:
On 2010/12/10 13:38:56, Vitaly wrote:
> Same here.

Done.