Bug 153379 - We should OSR exit with Int52Overflow when we fail to make an Int52 where we expect one.
Summary: We should OSR exit with Int52Overflow when we fail to make an Int52 where we ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
Depends on:
Blocks: 153019
  Show dependency treegraph
Reported: 2016-01-22 15:16 PST by Mark Lam
Modified: 2016-01-22 15:30 PST (History)
6 users (show)

See Also:

proposed patch. (8.16 KB, patch)
2016-01-22 15:21 PST, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff
x86_64 benchmark result. (65.31 KB, text/plain)
2016-01-22 15:23 PST, Mark Lam
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-01-22 15:16:28 PST
In DFG::Graph::addShouldSpeculateMachineInt(), we check !hasExitSite(add, Int52Overflow) when determining whether it's ok to speculate that an operand is of type Int52 or not.  However, the Int52Rep code that converts a double to Int52 will OSR exit with exit kind BadType instead.  This renders the hasExitSite() check in addShouldSpeculateMachineInt() useless.  This patch fixes this by changing Int52Rep to OSR exit with exit kind Int52Overflow instead when it fails to convert a double to an Int52.
Comment 1 Mark Lam 2016-01-22 15:21:10 PST
Created attachment 269617 [details]
proposed patch.
Comment 2 Mark Lam 2016-01-22 15:23:33 PST
Created attachment 269619 [details]
x86_64 benchmark result.

Perf is neutral so far with this patch.  None of the "definitely"s are consistently repeatable on retries.

Though perf is neutral in ToT, this will fix a bug that causes a perf regression in LongSpider crypto-aes when the patch for https://bugs.webkit.org/show_bug.cgi?id=153019 lands.
Comment 3 Filip Pizlo 2016-01-22 15:27:26 PST
Comment on attachment 269617 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=269617&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2217
> -            DFG_TYPE_CHECK(
> +            DFG_TYPE_CHECK_WITH_EXIT_KIND(Int52Overflow,
>                  JSValueRegs(), node->child1(), SpecInt52AsDouble,
>                  m_jit.branch64(
>                      JITCompiler::Equal, resultGPR,

Yuck!  But I get it.
Comment 4 Mark Lam 2016-01-22 15:30:56 PST
Thanks for the review.  Landed in r195488: <http://trac.webkit.org/r195488>.