Bug 104247 - Strange results calculating a square root in a loop
Summary: Strange results calculating a square root in a loop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.8
: P2 Normal
Assignee: Filip Pizlo
URL: http://jsfiddle.net/LgEDL/1/
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-12-06 03:01 PST by konstantin.k.ed
Modified: 2012-12-07 14:57 PST (History)
6 users (show)

See Also:


Attachments
same test as an attachment (1.14 KB, text/html)
2012-12-06 10:33 PST, Alexey Proskuryakov
no flags Details
fix the assertion (6.77 KB, patch)
2012-12-06 17:28 PST, Filip Pizlo
oliver: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description konstantin.k.ed 2012-12-06 03:01:36 PST
I'm computing the square root of an arbitrary number about a 1000 times and printing the result only if it's less than 0. At some point it starts printing 2.236.... (for sqrt of 5) which is the expected result, but shouldn't be printed since it's not a negative number. Furthermore, some iterations later it starts printing some large (negative for some inputs to sqrt) number.

I noticed it breaks for some numbers inside sqrt (5,7,11,15) but not for others (6, 13)
Comment 1 Alexey Proskuryakov 2012-12-06 10:33:18 PST
Created attachment 178028 [details]
same test as an attachment
Comment 2 Alexey Proskuryakov 2012-12-06 10:34:15 PST
Hits an assertion failure in debug ToT:

ASSERTION FAILED: originalNode->shouldGenerate()
/Users/ap/Safari/OpenSource/Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp(309) : void JSC::DFG::CFGSimplificationPhase::fixPossibleGetLocal(JSC::DFG::BasicBlock *, JSC::DFG::Edge &, bool)
1   0x10d872563 JSC::DFG::CFGSimplificationPhase::fixPossibleGetLocal(JSC::DFG::BasicBlock*, JSC::DFG::Edge&, bool)
2   0x10d870fb0 JSC::DFG::CFGSimplificationPhase::mergeBlocks(unsigned int, unsigned int, unsigned int)
3   0x10d86fb9e JSC::DFG::CFGSimplificationPhase::run()
4   0x10d86f8c5 bool JSC::DFG::runAndLog<JSC::DFG::CFGSimplificationPhase>(JSC::DFG::CFGSimplificationPhase&)
5   0x10d86f855 bool JSC::DFG::runPhase<JSC::DFG::CFGSimplificationPhase>(JSC::DFG::Graph&)
6   0x10d86f768 JSC::DFG::performCFGSimplification(JSC::DFG::Graph&)
Comment 3 Alexey Proskuryakov 2012-12-06 10:34:28 PST
<rdar://problem/12826880>
Comment 4 Filip Pizlo 2012-12-06 13:30:21 PST
Fascinating! It looks like this is somehow exposing a bug in our CFG simplifier. That's why the irrelevant branch is so important - without it we don't do CFG simplification, and presumably, don't end up breaking the IR leading to bad codegen.
Comment 5 Filip Pizlo 2012-12-06 17:28:22 PST
Created attachment 178118 [details]
fix the assertion
Comment 6 Filip Pizlo 2012-12-06 17:28:56 PST
(In reply to comment #5)
> Created an attachment (id=178118) [details]
> fix the assertion

I'm still running layout tests.  I will land once I have the -expected.txt file.
Comment 7 Oliver Hunt 2012-12-06 17:31:48 PST
Comment on attachment 178118 [details]
fix the assertion

r+, but you need to add the expected  results when you land
Comment 8 Build Bot 2012-12-06 20:59:28 PST
Comment on attachment 178118 [details]
fix the assertion

Attachment 178118 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15202006

New failing tests:
fast/js/dfg-cfg-simplify-redundant-dead-get-local.html
Comment 9 Filip Pizlo 2012-12-06 23:41:47 PST
(In reply to comment #7)
> (From update of attachment 178118 [details])
> r+, but you need to add the expected  results when you land

Yup, will do!
Comment 10 WebKit Review Bot 2012-12-07 01:39:12 PST
Comment on attachment 178118 [details]
fix the assertion

Attachment 178118 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15183315

New failing tests:
fast/js/dfg-cfg-simplify-redundant-dead-get-local.html
Comment 11 Filip Pizlo 2012-12-07 14:57:38 PST
Landed in http://trac.webkit.org/changeset/136989