RESOLVED FIXED 202072
[JSC] Int52Rep(DoubleRepAnyIntUse) should not call operation function 
https://bugs.webkit.org/show_bug.cgi?id=202072
Summary [JSC] Int52Rep(DoubleRepAnyIntUse) should not call operation function
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
DetailsFormatted DiffDiff
Patch (4.63 KB, patch)
2019-09-20 23:41 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (6.33 KB, patch)
2019-09-21 00:43 PDT, Yusuke Suzuki 		mark.lam: review+
DetailsFormatted DiffDiff
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.