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

Issue 163153002: introduce LTaggedToSmi (Closed)

Created:
6 years, 10 months ago by Weiliang
Modified:
6 years, 8 months ago
Reviewers:
Toon Verwaest
CC:
v8-dev
Base URL:
git://github.com/v8/v8.git@master
Visibility:
Public.

Description

introduce LTaggedToSmi prevent bailout for some non-smi cases BUG=

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -16 lines) Patch
M src/hydrogen-instructions.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/ia32/lithium-codegen-ia32.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/ia32/lithium-codegen-ia32.cc View 2 chunks +108 lines, -15 lines 0 comments Download
M src/ia32/lithium-ia32.h View 3 chunks +20 lines, -0 lines 0 comments Download
M src/ia32/lithium-ia32.cc View 1 chunk +8 lines, -1 line 0 comments Download

Messages

Total messages: 2 (0 generated)
Weiliang
6 years, 10 months ago (2014-02-13 08:41:06 UTC) #1
Toon Verwaest
6 years, 9 months ago (2014-03-11 13:45:36 UTC) #2
Not lgtm.

As discussed in a separate mail thread, I think this is the wrong approach. This
indicates that there are places in the code where we can't record type feedback
yet, but those values flow into phis with incompatible feedback. Eg, 

var v;
if (bool) {
  v = some_double_thats_never_used();
} else {
  v = some_smi;
}

if (!bool) { return v + 1; }

This will always deopt in CheckSmi in the first if(bool) branch, since that
double never goes into v+1, but the phi for v does. So we make the phi for v smi
since v + 1 has only seen smi. And we'll get a t->s change in that branch even
though it's a double. And we can never learn.

This approach works around it, but it actually does so invalidly. It converts
the double to a smi to avoid the deopt, but if the value is eg NaN, there is no
valid smi equivalent. So apart from somehow learning that v+1 should become
double, deopt is the only valid action.

I think the right fix is to record more type feedback. One place where that
should happen is return values of function calls. Another is context variable
accesses.

Powered by Google App Engine
This is Rietveld 408576698