Bug 133660 - [ftlopt] Call and Construct DFG nodes aren't always safe to execute
Summary: [ftlopt] Call and Construct DFG nodes aren't always safe to execute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matthew Mirman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-09 15:15 PDT by Matthew Mirman
Modified: 2014-06-11 17:25 PDT (History)
4 users (show)

See Also:


Attachments
Split Call and Construct DFG nodes into NativeCall and NativeConstruct (19.58 KB, patch)
2014-06-09 15:22 PDT, Matthew Mirman
fpizlo: review+
Details | Formatted Diff | Diff
Split Call and Construct DFG nodes into NativeCall and NativeConstruct (19.71 KB, patch)
2014-06-11 11:49 PDT, Matthew Mirman
fpizlo: review-
fpizlo: commit-queue-
Details | Formatted Diff | Diff
Split Call and Construct DFG nodes into NativeCall and NativeConstruct (19.90 KB, patch)
2014-06-11 15:45 PDT, Matthew Mirman
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Mirman 2014-06-09 15:15:52 PDT
Also needs native calling without patchpoints when not inlining in the ftl.
Comment 1 Matthew Mirman 2014-06-09 15:22:29 PDT
Created attachment 232738 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct

Ran webkit tests without new or relevant failures.

Should also increase the speed a bit by removing patch-points for calls which are speculated to be native by the DFG.
Comment 2 Filip Pizlo 2014-06-09 21:27:35 PDT
Nice!

I will land this shortly...
Comment 3 Filip Pizlo 2014-06-10 18:20:09 PDT
Sorry it's taking so long to land - I got some test failures, but I think they were spurious, so I'm rerunning...
Comment 4 Filip Pizlo 2014-06-10 18:52:42 PDT
Landed in http://trac.webkit.org/changeset/169786
Comment 5 Filip Pizlo 2014-06-10 21:30:21 PDT
Lol, this broke 32-bit.  I'll roll it out.
Comment 6 Filip Pizlo 2014-06-10 21:32:04 PDT
Rolled out in http://trac.webkit.org/changeset/169793
Comment 7 Filip Pizlo 2014-06-10 21:32:32 PDT
Comment on attachment 232738 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct

This is obsolete - this patch was landed but then rolled out because it broke internal run-javascriptcore-tests --32-bit.
Comment 9 Filip Pizlo 2014-06-11 00:01:35 PDT
(In reply to comment #8)
> and it made layout tests crash too on public Apple Mac bots:
> - http://build.webkit.org/builders/Apple%20Mavericks%20Release%20WK1%20%28Tests%29/builds/6543
> - http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/5806

That would be amazing since this was landed on a branch.
Comment 10 Csaba Osztrogonác 2014-06-11 00:03:56 PDT
(In reply to comment #9)
> That would be amazing since this was landed on a branch.
Good point. :) Sorry, I commented wrong bug report.
Comment 11 Matthew Mirman 2014-06-11 11:49:54 PDT
Created attachment 232879 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct 

Passed jsc tests on 32 bit and 64 bit.  Should be applied after https://bug-133718-attachments.webkit.org/attachment.cgi?id=232878
Comment 12 Filip Pizlo 2014-06-11 13:00:43 PDT
Comment on attachment 232879 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct 

View in context: https://bugs.webkit.org/attachment.cgi?id=232879&action=review

> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:261
> +        return false; // TODO: add a check for already checked. 

Can you file a bugzilla bug for this and reference it here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:644
> -    if (node->op() != Call)
> -        ASSERT(node->op() == Construct);
> +    bool isCall = node->op() == Call || node->op() == NativeCall;
> +    if (!isCall)
> +        ASSERT(node->op() == Construct || node->op() == NativeConstruct);

Why does this check for NativeCall/NativeConstruct?  The DFG backend shouldn't see them.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4131
> +    case NativeCall:
> +    case NativeConstruct:

Seems like these cases should be RELEASE_ASSERT_NOT_REACHED().

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:631
> -    if (node->op() != Call)
> -        RELEASE_ASSERT(node->op() == Construct);
> +
> +    bool isCall = node->op() == Call || node->op() == NativeCall;
> +    if (!isCall)
> +        RELEASE_ASSERT(node->op() == Construct || node->op() == NativeConstruct);

Why does this check for NativeCall/NativeConstruct?  The DFG backend shouldn't see them.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4163
> +    case NativeCall:
> +    case NativeConstruct:

Seems like these cases should be RELEASE_ASSERT_NOT_REACHED().
Comment 13 Matthew Mirman 2014-06-11 15:45:45 PDT
Created attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct
Comment 14 Filip Pizlo 2014-06-11 16:42:04 PDT
Comment on attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct 

Running tests on this now, will land shortly...
Comment 15 Filip Pizlo 2014-06-11 17:25:44 PDT
Landed in http://trac.webkit.org/changeset/169864