RESOLVED FIXED 174352
[FTL] Arguments elimination is suppressed by unreachable blocks
https://bugs.webkit.org/show_bug.cgi?id=174352
Summary [FTL] Arguments elimination is suppressed by unreachable blocks
Yusuke Suzuki
Reported 2017-07-11 03:30:42 PDT
I can see the case like this, 1: CreateClonedArguments ... 10: ForceOSRExit 11: GetById(@1, "length") In that case, even if 11 is not reachable from FTL code, our arguments elimination phase is suppressed by this, because our DFG constant folding's DFG node clipping is not executed yet.
Attachments
Patch (3.64 KB, patch)
2017-07-12 08:02 PDT, Yusuke Suzuki
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (1.73 MB, application/zip)
2017-07-12 16:06 PDT, Build Bot
no flags
Patch (8.43 KB, patch)
2017-07-14 01:09 PDT, Yusuke Suzuki
no flags
Patch (7.00 KB, patch)
2017-07-15 08:52 PDT, Yusuke Suzuki
no flags
Patch (6.51 KB, patch)
2017-07-15 09:00 PDT, Yusuke Suzuki
fpizlo: review+
buildbot: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.05 MB, application/zip)
2017-07-15 13:03 PDT, Build Bot
no flags
Patch for landing after stabilizing is done (6.64 KB, patch)
2017-07-16 02:25 PDT, Yusuke Suzuki
buildbot: commit-queue-
Archive of layout-test-results from ews125 for ios-simulator-wk2 (13.13 MB, application/zip)
2017-07-16 03:54 PDT, Build Bot
no flags
Archive of layout-test-results from ews100 for mac-elcapitan (1.14 MB, application/zip)
2017-07-16 03:59 PDT, Build Bot
no flags
Archive of layout-test-results from ews112 for mac-elcapitan (1.92 MB, application/zip)
2017-07-16 04:41 PDT, Build Bot
no flags
Patch for landing after stabilizing is done (8.15 KB, patch)
2017-07-16 12:54 PDT, Yusuke Suzuki
no flags
Yusuke Suzuki
Comment 1 2017-07-11 03:32:25 PDT
Let's see this effect with mathjs https://github.com/josdejong/mathjs ``` function runMathjs(n) { const a = math.zeros(n, n) const b = math.zeros(n, n) for (let i = 0; i < n; ++i) { for (let j = 0; j < n; ++j) { a.set([i, j], Math.random()) b.set([i, j], Math.random()) } } for (var i = 0; i < 30; ++i) { math.multiply(a, b) } } runMathjs(100); ``` This is super slow in JSC because arguments elimination is suppressed in add#AebcRZ.
Yusuke Suzuki
Comment 2 2017-07-12 08:02:09 PDT
Created attachment 315240 [details] Patch WIP
Build Bot
Comment 3 2017-07-12 16:06:31 PDT
Comment on attachment 315240 [details] Patch Attachment 315240 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4109675 New failing tests: storage/indexeddb/modern/new-database-after-user-delete.html
Build Bot
Comment 4 2017-07-12 16:06:32 PDT
Created attachment 315298 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Yusuke Suzuki
Comment 5 2017-07-14 01:09:00 PDT
Saam Barati
Comment 6 2017-07-14 11:10:28 PDT
Comment on attachment 315410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315410&action=review > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:414 > + case GetById: > + // If we already perform OSR-exit, GetById can be simply ignored. > + // This can happen if you have a basic block that is not executed yet and references elimination candidates. > + // Since this GetById code is not executed yet, it is always GetById (neither GetByOffset nor GetByIdFlush). > + // We do not need to consider GetByIdFlush here. > + // We focus on ForceOSRExit. It is OK because explicit OSR exit nodes like Unreachable, Throw etc. are terminal > + // nodes. So it does not appear in the middle of a basic block. > + if (isOSRExited) > + break; > + m_graph.doToChildren(node, [&] (Edge edge) { return escape(edge, node); }); > + break; We can't just special case GetById. There are plenty of other operations that might escape an allocation. Regarding your boolean here, why not just break out of walking over this block when seeing a node that is an exit. I would just add something like: case ForceOSRExit: case CheckBadCell: case Throw: case ThrowStaticError: break out of for loop. > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:1118 > + isOSRExited |= node->isPseudoTerminal(); and then I'd just add the same cases to this switch statement, and break out early. > Source/JavaScriptCore/dfg/DFGNode.h:1313 > + // As is described in DFGNodeType.h's ForceOSRExit, this is a pseudo-terminal. > + // It means that execution should fall out of DFG at this point, but execution > + // does continue in the basic block - just in a different compiler. > + bool isPseudoTerminal() I wouldn't add this function. This is nicer just in the arguments elimination phase as part of its switch statements. > Source/JavaScriptCore/dfg/DFGValidate.cpp:737 > + case GetById: > + if (isOSRExited) > + break; > + FALLTHROUGH; ditto here.
Yusuke Suzuki
Comment 7 2017-07-15 08:52:02 PDT
Comment on attachment 315410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315410&action=review >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:414 >> + break; > > We can't just special case GetById. There are plenty of other operations that might escape an allocation. > > Regarding your boolean here, why not just break out of walking over this block when seeing a node that is an exit. > I would just add something like: > case ForceOSRExit: > case CheckBadCell: > case Throw: > case ThrowStaticError: > break out of for loop. I'm a bit worried about that XXXNode(PhantomClonedArguments) node appears in lowering phase. But since we cannot hoist nodes over ForceOSRExit, the above thing cannot happen. I'll change this to if (node->isPseudoTerminal()) break; since pseudo terminal nodes may escape (currently there is no such nodes). >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:1118 >> + isOSRExited |= node->isPseudoTerminal(); > > and then I'd just add the same cases to this switch statement, and break out early. Changed this to if (node->isPseudoTerminal()) break; >> Source/JavaScriptCore/dfg/DFGNode.h:1313 >> + bool isPseudoTerminal() > > I wouldn't add this function. This is nicer just in the arguments elimination phase as part of its switch statements. I think this is useful since we use this too in DFGValidate. What do you think of?
Yusuke Suzuki
Comment 8 2017-07-15 08:52:52 PDT
Yusuke Suzuki
Comment 9 2017-07-15 09:00:43 PDT
Filip Pizlo
Comment 10 2017-07-15 09:41:17 PDT
Comment on attachment 315547 [details] Patch Can you add a fixme above the declaration of isPseudoTerminal that says something like that we should consider replacing all users of this with AI-based reachablity?
Build Bot
Comment 11 2017-07-15 13:03:30 PDT
Comment on attachment 315547 [details] Patch Attachment 315547 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4126762 New failing tests: http/tests/canvas/canvas-tainted-after-draw-image.html
Build Bot
Comment 12 2017-07-15 13:03:32 PDT
Created attachment 315564 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Yusuke Suzuki
Comment 13 2017-07-16 02:24:11 PDT
(In reply to Filip Pizlo from comment #10) > Comment on attachment 315547 [details] > Patch > > Can you add a fixme above the declaration of isPseudoTerminal that says > something like that we should consider replacing all users of this with > AI-based reachablity? Thanks, added.
Yusuke Suzuki
Comment 14 2017-07-16 02:25:20 PDT
Created attachment 315598 [details] Patch for landing after stabilizing is done Patch for landing after stabilizing is done
Build Bot
Comment 15 2017-07-16 03:54:04 PDT
Comment on attachment 315598 [details] Patch for landing after stabilizing is done Attachment 315598 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4129991 New failing tests: http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html
Build Bot
Comment 16 2017-07-16 03:54:22 PDT
Created attachment 315600 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Build Bot
Comment 17 2017-07-16 03:59:04 PDT
Comment on attachment 315598 [details] Patch for landing after stabilizing is done Attachment 315598 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4130072 New failing tests: security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html
Build Bot
Comment 18 2017-07-16 03:59:05 PDT
Created attachment 315601 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 19 2017-07-16 04:41:38 PDT
Comment on attachment 315598 [details] Patch for landing after stabilizing is done Attachment 315598 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4130156 New failing tests: security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html
Build Bot
Comment 20 2017-07-16 04:41:39 PDT
Created attachment 315602 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Saam Barati
Comment 21 2017-07-16 11:49:45 PDT
Comment on attachment 315598 [details] Patch for landing after stabilizing is done View in context: https://bugs.webkit.org/attachment.cgi?id=315598&action=review > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:412 > + if (node->isPseudoTerminal()) Can you add a test where we “throw arguments” just to ensure that this break is in the correct place?
Yusuke Suzuki
Comment 22 2017-07-16 12:47:29 PDT
Comment on attachment 315598 [details] Patch for landing after stabilizing is done View in context: https://bugs.webkit.org/attachment.cgi?id=315598&action=review >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:412 >> + if (node->isPseudoTerminal()) > > Can you add a test where we “throw arguments” just to ensure that this break is in the correct place? Sure!
Yusuke Suzuki
Comment 23 2017-07-16 12:54:53 PDT
Created attachment 315619 [details] Patch for landing after stabilizing is done Patch for landing
Yusuke Suzuki
Comment 24 2017-07-21 09:01:12 PDT
Note You need to log in before you can comment on or make changes to this bug.