Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could round rather than truncate in compileClampDoubleToByte? #16609

Conversation

kmiller68
Copy link
Contributor

@kmiller68 kmiller68 commented Aug 11, 2023

c55d067

Could round rather than truncate in compileClampDoubleToByte?
https://bugs.webkit.org/show_bug.cgi?id=72054
rdar://113734964

Reviewed by Yusuke Suzuki.

Right now there's a difference between our non-optimizing code and our C++ code for ClampedUint8Arrays.
In the C++ code we correctly do a round to nearest (ties to even) but in the optimizing JITs we do a round to inifinity.
This patch fixes our optimizing code to round to nearest too.

* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::clampDoubleToByte):
(JSC::DFG::compileClampDoubleToByte):
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):

Canonical link: https://commits.webkit.org/267100@main

ee94e3c

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ›  jsc-armv7
βœ… πŸ›  tv-sim ❌ πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch ❌ πŸ›  jsc-mips
βœ… πŸ›  watch-sim ❌ πŸ§ͺ jsc-mips-tests

@kmiller68 kmiller68 requested a review from a team as a code owner August 11, 2023 17:34
@kmiller68 kmiller68 self-assigned this Aug 11, 2023
@kmiller68 kmiller68 added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Aug 11, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 11, 2023
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@kmiller68 kmiller68 removed the merging-blocked Applied to prevent a change from being merged label Aug 11, 2023
@kmiller68 kmiller68 force-pushed the eng/Could-round-rather-than-truncate-in-compileClampDoubleToByte branch from 88ed1fc to ee94e3c Compare August 11, 2023 18:03
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 11, 2023
@kmiller68 kmiller68 added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Aug 21, 2023
@kmiller68
Copy link
Contributor Author

Merging despite the build/test failures for mips and armv7 per request of @jjgriego

https://bugs.webkit.org/show_bug.cgi?id=72054
rdar://113734964

Reviewed by Yusuke Suzuki.

Right now there's a difference between our non-optimizing code and our C++ code for ClampedUint8Arrays.
In the C++ code we correctly do a round to nearest (ties to even) but in the optimizing JITs we do a round to inifinity.
This patch fixes our optimizing code to round to nearest too.

* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::clampDoubleToByte):
(JSC::DFG::compileClampDoubleToByte):
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):

Canonical link: https://commits.webkit.org/267100@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Could-round-rather-than-truncate-in-compileClampDoubleToByte branch from ee94e3c to c55d067 Compare August 21, 2023 18:56
@webkit-commit-queue
Copy link
Collaborator

Committed 267100@main (c55d067): https://commits.webkit.org/267100@main

Reviewed commits have been landed. Closing PR #16609 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit c55d067 into WebKit:main Aug 21, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
5 participants