Bug 196712 - Modify how we do SetArgument when we inline varargs calls
Summary: Modify how we do SetArgument when we inline varargs calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 196828
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-08 15:02 PDT by Saam Barati
Modified: 2019-04-15 19:41 PDT (History)
15 users (show)

See Also:


Attachments
WIP (2.52 KB, patch)
2019-04-08 19:38 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (3.78 KB, patch)
2019-04-08 22:20 PDT, Saam Barati
ews: commit-queue-
Details | Formatted Diff | Diff
WIP (8.58 KB, patch)
2019-04-09 01:24 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (31.25 KB, patch)
2019-04-11 16:17 PDT, Saam Barati
msaboff: review+
Details | Formatted Diff | Diff
patch for landing (31.24 KB, patch)
2019-04-15 18:11 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-04-08 15:02:14 PDT
...
Comment 1 Filip Pizlo 2019-04-08 15:24:19 PDT
wait, GetStack has children?
Comment 2 Filip Pizlo 2019-04-08 15:25:32 PDT
Also... this smells like a safeToExecute situation.
Comment 3 Saam Barati 2019-04-08 16:15:59 PDT
        for (unsigned i = data->limit - 1; i--;)
=>
        for (unsigned i = data->limit; i--;)
Comment 4 Saam Barati 2019-04-08 16:16:45 PDT
<rdar://problem/49605012>
Comment 5 Saam Barati 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.
Comment 6 Saam Barati 2019-04-08 19:38:25 PDT
Created attachment 367017 [details]
WIP

Currently fails a bunch of tests
Comment 7 Saam Barati 2019-04-08 22:20:07 PDT
Created attachment 367028 [details]
WIP

test on EWS
Comment 8 Build Bot 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
Comment 9 Saam Barati 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.
Comment 10 Build Bot 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.
Comment 11 Saam Barati 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.
Comment 12 Saam Barati 2019-04-11 16:17:04 PDT
Created attachment 367260 [details]
patch
Comment 13 Michael Saboff 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*?
Comment 14 Saam Barati 2019-04-15 18:11:53 PDT
Created attachment 367484 [details]
patch for landing

Thanks for the review.
Comment 15 WebKit Commit Bot 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>
Comment 16 WebKit Commit Bot 2019-04-15 19:41:46 PDT
All reviewed patches have been landed.  Closing bug.