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

Issue 2814013003: [turbofan] Properly represent the float64 hole. (Closed)

Created:
3 years, 8 months ago by Benedikt Meurer
Modified:
3 years, 8 months ago
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/heads/master
Project:
v8
Visibility:
Public.

Description

[turbofan] Properly represent the float64 hole. The hole NaN should also have proper Type::Hole, and not silently hide in the Type::Number. This way we can remove all the special casing for the hole NaN, and we also finally get the CheckNumber right. This also allows us to remove some ducktape from the Deoptimizer, as for escape analyzed FixedDoubleArrays we always pass the hole value now to represent the actual holes. Also-By: jarin@chromium.org BUG=chromium:684208, chromium:709753, v8:5267 R=jarin@chromium.org Review-Url: https://codereview.chromium.org/2814013003 Cr-Commit-Position: refs/heads/master@{#44603} Committed: https://chromium.googlesource.com/v8/v8/+/8c0c5e8117a0c935d6f2e5f6e540674d46753a87

Patch Set 1 #

Patch Set 2 : CheckNumber does not need a restriction type. #

Patch Set 3 : Make sure the_hole has the correct hole NaN bit pattern on Win32. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -56 lines) Patch
M src/compiler/js-create-lowering.cc View 2 chunks +2 lines, -21 lines 0 comments Download
M src/compiler/js-native-context-specialization.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M src/compiler/operation-typer.h View 1 chunk +4 lines, -0 lines 0 comments Download
M src/compiler/operation-typer.cc View 1 chunk +13 lines, -0 lines 0 comments Download
M src/compiler/simplified-lowering.cc View 1 4 chunks +54 lines, -20 lines 2 comments Download
M src/compiler/typed-optimization.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/typed-optimization.cc View 2 chunks +12 lines, -0 lines 0 comments Download
M src/compiler/typer.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M src/compiler/types.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/verifier.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M src/deoptimizer.cc View 2 chunks +2 lines, -6 lines 0 comments Download
M src/objects.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/objects.cc View 1 2 1 chunk +6 lines, -1 line 0 comments Download
M src/objects-inl.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A test/mjsunit/regress/regress-crbug-709753.js View 1 chunk +13 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (14 generated)
Benedikt Meurer
3 years, 8 months ago (2017-04-12 07:28:38 UTC) #1
Benedikt Meurer
Here's the proper fix, based on pair programming approach! :-)
3 years, 8 months ago (2017-04-12 07:29:41 UTC) #5
Jarin
lgtm
3 years, 8 months ago (2017-04-12 07:38:17 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2814013003/1
3 years, 8 months ago (2017-04-12 07:38:43 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/builds/4243) v8_linux_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, ...
3 years, 8 months ago (2017-04-12 08:05:59 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2814013003/20001
3 years, 8 months ago (2017-04-12 08:17:39 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/26060) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, ...
3 years, 8 months ago (2017-04-12 09:02:23 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2814013003/40001
3 years, 8 months ago (2017-04-12 09:42:30 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/8c0c5e8117a0c935d6f2e5f6e540674d46753a87
3 years, 8 months ago (2017-04-12 10:10:56 UTC) #22
Michael Achenbach
https://codereview.chromium.org/2814013003/diff/40001/src/compiler/simplified-lowering.cc File src/compiler/simplified-lowering.cc (right): https://codereview.chromium.org/2814013003/diff/40001/src/compiler/simplified-lowering.cc#newcode2703 src/compiler/simplified-lowering.cc:2703: if (truncation.IsUsedAsWord32()) { FYI: According to (64 bit) coverage ...
3 years, 8 months ago (2017-04-12 10:23:01 UTC) #24
Benedikt Meurer
3 years, 8 months ago (2017-04-12 10:24:18 UTC) #25
Message was sent while issue was closed.
https://codereview.chromium.org/2814013003/diff/40001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2814013003/diff/40001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:2703: if (truncation.IsUsedAsWord32()) {
Not necessarily.

Powered by Google App Engine
This is Rietveld 408576698