RESOLVED FIXED 196712
Modify how we do SetArgument when we inline varargs calls
https://bugs.webkit.org/show_bug.cgi?id=196712
Summary Modify how we do SetArgument when we inline varargs calls
Saam Barati
Reported 2019-04-08 15:02:14 PDT
...
Attachments
WIP (2.52 KB, patch)
2019-04-08 19:38 PDT, Saam Barati
no flags
WIP (3.78 KB, patch)
2019-04-08 22:20 PDT, Saam Barati
ews-watchlist: commit-queue-
WIP (8.58 KB, patch)
2019-04-09 01:24 PDT, Saam Barati
no flags
patch (31.25 KB, patch)
2019-04-11 16:17 PDT, Saam Barati
msaboff: review+
patch for landing (31.24 KB, patch)
2019-04-15 18:11 PDT, Saam Barati
no flags
Filip Pizlo
Comment 1 2019-04-08 15:24:19 PDT
wait, GetStack has children?
Filip Pizlo
Comment 2 2019-04-08 15:25:32 PDT
Also... this smells like a safeToExecute situation.
Saam Barati
Comment 3 2019-04-08 16:15:59 PDT
for (unsigned i = data->limit - 1; i--;) => for (unsigned i = data->limit; i--;)
Saam Barati
Comment 4 2019-04-08 16:16:45 PDT
Saam Barati
Comment 5 2019-04-08 16:21:08 PDT
(In reply to Saam Barati from comment #3) > for (unsigned i = data->limit - 1; i--;) > => > for (unsigned i = data->limit; i--;) Interestingly this isn't the fix. But it also looks wrong.
Saam Barati
Comment 6 2019-04-08 19:38:25 PDT
Created attachment 367017 [details] WIP Currently fails a bunch of tests
Saam Barati
Comment 7 2019-04-08 22:20:07 PDT
Created attachment 367028 [details] WIP test on EWS
EWS Watchlist
Comment 8 2019-04-09 00:43:33 PDT
Comment on attachment 367028 [details] WIP Attachment 367028 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/11815225 New failing tests: stress/tailCallForwardArguments.js.dfg-eager-no-cjit-validate stress/tailCallForwardArguments.js.ftl-eager stress/tailCallForwardArguments.js.ftl-eager-no-cjit-b3o1 v8-v6/v8-raytrace.js.ftl-eager
Saam Barati
Comment 9 2019-04-09 01:24:20 PDT
Created attachment 367034 [details] WIP See if this works on EWS. I'm thinking perhaps we really want to keep these SetArgument nodes and just mark the ones from mandatoryMinimum to limit to not be lowered to GetStack in SSA conversion.
EWS Watchlist
Comment 10 2019-04-09 01:26:45 PDT
Attachment 367034 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1861: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:412: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4347: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4349: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4358: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4360: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1870: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1872: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 8 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 11 2019-04-11 16:15:33 PDT
(In reply to Saam Barati from comment #3) > for (unsigned i = data->limit - 1; i--;) > => > for (unsigned i = data->limit; i--;) FWIW, this isn't an issue. We're doing the right thing here. The bug is actually in clobberize, we claim to write an extra stack slot.
Saam Barati
Comment 12 2019-04-11 16:17:04 PDT
Michael Saboff
Comment 13 2019-04-15 15:04:10 PDT
Comment on attachment 367260 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=367260&action=review r=me > Source/JavaScriptCore/ChangeLog:15 > + However, we used to always to emit SetArgumentDefinitely up to "limit - 1" for Awkward wording. How about "However, we used to always emit ..." > Source/JavaScriptCore/ChangeLog:16 > + all are arguments, even when some arguments aren't guaranteed to be in a valid Delete *are*?
Saam Barati
Comment 14 2019-04-15 18:11:53 PDT
Created attachment 367484 [details] patch for landing Thanks for the review.
WebKit Commit Bot
Comment 15 2019-04-15 19:41:44 PDT
Comment on attachment 367484 [details] patch for landing Clearing flags on attachment: 367484 Committed r244324: <https://trac.webkit.org/changeset/244324>
WebKit Commit Bot
Comment 16 2019-04-15 19:41:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.