Bug 202072

Summary:

[JSC] Int52Rep(DoubleRepAnyIntUse) should not call operation function

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

JavaScriptCore

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	mark.lam: review+

Yusuke Suzuki

Reported 2019-09-20 22:11:55 PDT

operationConvertDoubleToInt52 is super simple, we should do it in FTL's B3.

Attachments
Patch (5.23 KB, patch) 2019-09-20 23:28 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (4.63 KB, patch) 2019-09-20 23:41 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (6.33 KB, patch) 2019-09-21 00:43 PDT, Yusuke Suzuki	mark.lam: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-09-20 22:13:23 PDT

<rdar://problem/55582459>

Yusuke Suzuki

Comment 2 2019-09-20 23:28:58 PDT

Created attachment 379310 [details] Patch

Yusuke Suzuki

Comment 3 2019-09-20 23:41:16 PDT

Created attachment 379311 [details] Patch

Yusuke Suzuki

Comment 4 2019-09-20 23:46:22 PDT

This change improves JetStream2/stanford-crypto-sha256 by ~5%.

Yusuke Suzuki

Comment 5 2019-09-21 00:43:04 PDT

Created attachment 379315 [details] Patch

Mark Lam

Comment 6 2019-09-21 09:08:26 PDT

Comment on attachment 379315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379315&action=review r=me > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15909 > + speculate(Int52Overflow, doubleValue(value), edge.node(), m_out.greaterThanOrEqual(integerValue, m_out.constInt64(static_cast<int64_t>(1) << (JSValue::numberOfInt52Bits - 1)))); > + speculate(Int52Overflow, doubleValue(value), edge.node(), m_out.lessThan(integerValue, m_out.constInt64(-(static_cast<int64_t>(1) << (JSValue::numberOfInt52Bits - 1))))); This looks correct to me, but please also add some tests to verify that these edge cases are handled correctly ... just to make sure we don't have an off by 1 error.

Mark Lam

Comment 7 2019-09-21 09:09:34 PDT

Comment on attachment 379315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379315&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15894 > + speculate(Int52Overflow, doubleValue(value), edge.node(), valueNotConvertibleToInteger); Might as well add a test case for this condition too. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15904 > + speculate(Int52Overflow, doubleValue(value), edge.node(), signBitSet); Might as well add a test case for this condition too.

Yusuke Suzuki

Comment 8 2019-09-22 02:45:10 PDT

Comment on attachment 379315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379315&action=review Thanks! >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15894 >> + speculate(Int52Overflow, doubleValue(value), edge.node(), valueNotConvertibleToInteger); > > Might as well add a test case for this condition too. Added. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15904 >> + speculate(Int52Overflow, doubleValue(value), edge.node(), signBitSet); > > Might as well add a test case for this condition too. Added. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15909 >> + speculate(Int52Overflow, doubleValue(value), edge.node(), m_out.lessThan(integerValue, m_out.constInt64(-(static_cast<int64_t>(1) << (JSValue::numberOfInt52Bits - 1))))); > > This looks correct to me, but please also add some tests to verify that these edge cases are handled correctly ... just to make sure we don't have an off by 1 error. Added.

Yusuke Suzuki

Comment 9 2019-09-22 02:57:52 PDT

Committed r250200: <https://trac.webkit.org/changeset/250200>

Note You need to log in before you can comment on or make changes to this bug.