-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
deps: V8: cherry-pick 56f6a76 #25269
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Original commit message: [turbofan] Fix -0 check for subnormals. Previously we'd check `x` for -0 by testing `(1.0 / x) == -Infinity`, but this will yield the wrong results when `x` is a subnormal, i.e. really close to 0. In CSA we already perform bit checks to test for -0, so teach TurboFan to do the same for comparisons to -0 (via `Object.is`). We introduce a new NumberIsMinusZero simplified operator to handle the case where SimplifiedLowering already knows that the input is a number. Bug: chromium:903043, v8:6882 Change-Id: I0cb7c568029b461a92fc183104d5f359b4bfe7f4 Reviewed-on: https://chromium-review.googlesource.com/c/1328802 Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Reviewed-by: Sigurd Schneider <sigurds@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#57382} Refs: v8/v8@56f6a76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber-stamp LGTM.
just as a funny sidenote, we made the same mistake in node core :p #17507 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber stamp LGTM
Resumed yellow CI https://ci.nodejs.org/job/node-test-pull-request/20008/ |
Original commit message: [turbofan] Fix -0 check for subnormals. Previously we'd check `x` for -0 by testing `(1.0 / x) == -Infinity`, but this will yield the wrong results when `x` is a subnormal, i.e. really close to 0. In CSA we already perform bit checks to test for -0, so teach TurboFan to do the same for comparisons to -0 (via `Object.is`). We introduce a new NumberIsMinusZero simplified operator to handle the case where SimplifiedLowering already knows that the input is a number. Bug: chromium:903043, v8:6882 Change-Id: I0cb7c568029b461a92fc183104d5f359b4bfe7f4 Reviewed-on: https://chromium-review.googlesource.com/c/1328802 Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Reviewed-by: Sigurd Schneider <sigurds@chromium.org> Cr-Commit-Position: refs/heads/master@{#57382} PR-URL: #25269 Refs: v8/v8@56f6a76 Fixes: #25268 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR This would need a manual backport to v11.x-staging (it applies cleanly but does not compile) |
@addaleax I would like to backport this and it lands cleanly on v11.x but it relies on some other code and I do not know the code base well enough to backport this on my own (see also #25270 (comment)). It would be great if the @nodejs/v8-update could have a look at backporting this. |
Attempts to backport to v11 and v10: |
Original commit message:
Refs: v8/v8@56f6a76
Fixes: #25268
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes