Bug 133660 – [ftlopt] Call and Construct DFG nodes aren't always safe to execute

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Matthew Mirman]

Also needs native calling without patchpoints when not inlining in the ftl.

[Matthew Mirman]

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.

[Filip Pizlo]

Nice!

I will land this shortly...

[Filip Pizlo]

Sorry it's taking so long to land - I got some test failures, but I think they were spurious, so I'm rerunning...

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/169786

[Filip Pizlo]

Lol, this broke 32-bit.  I'll roll it out.

[Filip Pizlo]

Rolled out in http://trac.webkit.org/changeset/169793

[Filip Pizlo]

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.

[Csaba Osztrogonác]

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

[Filip Pizlo]

(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.

[Csaba Osztrogonác]

(In reply to comment #9)
> That would be amazing since this was landed on a branch.
Good point. :) Sorry, I commented wrong bug report.

[Matthew Mirman]

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

[Filip Pizlo]

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().

[Matthew Mirman]

Created attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct

[Filip Pizlo]

Comment on attachment 232917 [details]
Split Call and Construct DFG nodes into NativeCall and NativeConstruct 

Running tests on this now, will land shortly...

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/169864