Patch forthcoming.
Created attachment 281595 [details] something like this
Created attachment 281620 [details] maybe the patch This does the trick. What's sad about this is that this optimization really belongs in lower tiers, but currently it's not feasible to do it in DFG IR. You really need B3 IR to do it. But without the optimization, the code runs so incredibly slow that it takes a long time to get to FTL. It's starting to become quite clear that we need tier-up checks in straight-line code.
Created attachment 281622 [details] better patch
Looks like this has a bug: stress/float64-array-nan-inlined.js.default: test_script_12303: line 2: 55586 Segmentation fault: 11 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true float64-array-nan-inlined.js ) stress/float64-array-nan-inlined.js.default: ERROR: Unexpected exit code: 139 Maybe it's latent so I'm still testing without the patch.
(In reply to comment #4) > Looks like this has a bug: > > stress/float64-array-nan-inlined.js.default: test_script_12303: line 2: > 55586 Segmentation fault: 11 ( "$@" > ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false > --useFunctionDotArguments\=true float64-array-nan-inlined.js ) > stress/float64-array-nan-inlined.js.default: ERROR: Unexpected exit code: 139 > > Maybe it's latent so I'm still testing without the patch. Wait, it has to be latent! It's a non-FTL test and this patch only changes FTL.
(In reply to comment #5) > (In reply to comment #4) > > Looks like this has a bug: > > > > stress/float64-array-nan-inlined.js.default: test_script_12303: line 2: > > 55586 Segmentation fault: 11 ( "$@" > > ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false > > --useFunctionDotArguments\=true float64-array-nan-inlined.js ) > > stress/float64-array-nan-inlined.js.default: ERROR: Unexpected exit code: 139 > > > > Maybe it's latent so I'm still testing without the patch. > > Wait, it has to be latent! It's a non-FTL test and this patch only changes > FTL. Yeah, it was an unrelated bug, and I fixed it.
Created attachment 282086 [details] awesome patch
Created attachment 282087 [details] patch
Created attachment 283979 [details] the patch
Created attachment 283980 [details] the patch
Comment on attachment 283980 [details] the patch Oh man, this doesn't quite work! Fixing...
Created attachment 283981 [details] the patch
Comment on attachment 283981 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=283981&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:439 > + // put them. We're not actually smart enough things to move things around at random. "smart enough things" => "smart enough" > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:466 > + if (m_blockInsertionSet.execute() | m_changedCFG) { Should be "||"
(In reply to comment #13) > Comment on attachment 283981 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283981&action=review > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:439 > > + // put them. We're not actually smart enough things to move things around at random. > > "smart enough things" => "smart enough" Fixed. > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:466 > > + if (m_blockInsertionSet.execute() | m_changedCFG) { > > Should be "||" That was sort of deliberate. I wanted to emphasize that every subexpression should execute for side-effects. I changed it to: m_changedCFG |= m_blockInsertionSet.execute(); if (m_changedCFG) {
Created attachment 284037 [details] the patch
Created attachment 284059 [details] performance
Comment on attachment 284037 [details] the patch r=me. Out of curiosity does this optimization kick in if the user doesn't write a switch explicitly? If so, maybe it would be worth adding a test. For example: if (a === sym1) ... if (a === sym2) ... ...
(In reply to comment #17) > Comment on attachment 284037 [details] > the patch > > r=me. Out of curiosity does this optimization kick in if the user doesn't > write a switch explicitly? If so, maybe it would be worth adding a test. For > example: > > if (a === sym1) > ... > if (a === sym2) > ... > ... It does kick in for this! Good idea, I will add a test for it.
(In reply to comment #18) > (In reply to comment #17) > > Comment on attachment 284037 [details] > > the patch > > > > r=me. Out of curiosity does this optimization kick in if the user doesn't > > write a switch explicitly? If so, maybe it would be worth adding a test. For > > example: > > > > if (a === sym1) > > ... > > if (a === sym2) > > ... > > ... > > It does kick in for this! Good idea, I will add a test for it. Yeah, I wrote a test like this - basically a scripted rewrite of bigswitch-indirect-symbol to use if - and it gets a speed-up from this patch.
Landed in https://trac.webkit.org/changeset/203491