WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
WIP
(3.78 KB, patch)
2019-04-08 22:20 PDT
,
Saam Barati
ews-watchlist
: 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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/49605012
>
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
Created
attachment 367260
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug