Chromium Code Reviews
Help | Chromium Project | Sign in
(326)

Issue 5714001: Improve our type feedback by recogizining never-executed IC calls for binary ... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 4 months ago by fschneider
Modified:
2 years, 11 months ago
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 11

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 16

Patch Set 5 : '' #

Patch Set 6 : fix lintos #

Patch Set 7 : address comments, fixed ARM build. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+202 lines, -61 lines) Lint Patch
M src/arm/ic-arm.cc View 1 2 3 4 5 6 2 chunks +7 lines, -3 lines 0 comments 0 errors Download
M src/full-codegen.h View 1 2 3 4 5 6 2 chunks +7 lines, -0 lines 0 comments 1 errors Download
M src/ia32/assembler-ia32.h View 1 2 3 1 chunk +8 lines, -0 lines 0 comments 0 errors Download
M src/ia32/code-stubs-ia32.h View 1 2 3 2 chunks +3 lines, -3 lines 0 comments 0 errors Download
M src/ia32/code-stubs-ia32.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments 0 errors Download
M src/ia32/full-codegen-ia32.cc View 1 2 3 4 5 6 20 chunks +114 lines, -46 lines 0 comments 0 errors Download
M src/ia32/ic-ia32.cc View 1 2 3 2 chunks +36 lines, -3 lines 0 comments 0 errors Download
M src/ia32/lithium-codegen-ia32.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments 0 errors Download
M src/ic.h View 1 2 3 2 chunks +3 lines, -1 line 0 comments 0 errors Download
M src/ic.cc View 1 2 3 4 5 3 chunks +9 lines, -3 lines 0 comments 0 errors Download
M src/type-info.cc View 1 2 3 4 5 6 3 chunks +6 lines, -0 lines 0 comments 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 8
fschneider
Please have a look.
3 years, 4 months ago #1
fschneider
Adding Vitaly. This may also be useful to detect more call sites that are never ...
3 years, 4 months ago #2
Vitaly Repeshko
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.cc#newcode1557 src/ia32/full-codegen-ia32.cc:1557: masm_->bind(&patch_site_); In debug mode in ~JumpPatchSite we could ...
3 years, 4 months ago #3
William Hesse
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#newcode576 src/ia32/assembler-ia32.h:576: static const byte kJmpShortOpcode = 0xeb; I think ...
3 years, 4 months ago #4
Kevin Millikin
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.cc#newcode1597 src/ia32/full-codegen-ia32.cc:1597: __ CallStub(&stub); I like Vitaly's idea because it's safer. ...
3 years, 4 months ago #5
fschneider
Thanks for the comments. I uploaded a new version which does the same for CompareIC. ...
3 years, 4 months ago #6
Vitaly Repeshko
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.cc#newcode47 src/ia32/full-codegen-ia32.cc:47: JumpPatchSite(MacroAssembler* masm) : masm_(masm), info_emitted_(false) { } ...
3 years, 4 months ago #7
fschneider
3 years, 4 months ago #8
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6