Bug 159786 – DFG and FTL should support op_call_eval

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

[Filip Pizlo]

Because it's part of the language.

[Filip Pizlo]

Created attachment 283688 [details]
work in progress

[Filip Pizlo]

Created attachment 283690 [details]
maybe it's done

[Filip Pizlo]

Created attachment 283700 [details]
it just did an eval!

[Filip Pizlo]

Created attachment 283715 [details]
now it works in FTL, too

Still haven't run all tests though.

[WebKit Commit Bot]

Attachment 283715 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5563:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5588:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:885:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:622:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:842:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 283715 [details]
now it works in FTL, too

Attachment 283715 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1682847

New failing tests:
js/regress/eval-not-eval-compute-args.html
js/regress/eval-not-eval-compute.html

[Build Bot]

Created attachment 283723 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283715 [details]
now it works in FTL, too

Attachment 283715 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1682886

New failing tests:
js/regress/eval-not-eval-compute-args.html
js/regress/eval-not-eval-compute.html

[Build Bot]

Created attachment 283724 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283715 [details]
now it works in FTL, too

Attachment 283715 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1682893

New failing tests:
js/regress/eval-not-eval-compute-args.html
js/regress/eval-not-eval-compute.html

[Build Bot]

Comment on attachment 283715 [details]
now it works in FTL, too

Attachment 283715 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1682888

New failing tests:
js/regress/function-with-eval.html
js/regress/eval-not-eval-compute-args.html
js/regress/eval-not-eval-compute.html

[Build Bot]

Created attachment 283727 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Created attachment 283728 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Created attachment 283734 [details]
new and improved!

[WebKit Commit Bot]

Attachment 283734 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5563:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5588:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:885:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:622:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:842:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 283736 [details]
maybe it's done

[WebKit Commit Bot]

Attachment 283736 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5563:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5588:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:885:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:622:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:842:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 283736 [details]
maybe it's done

Attachment 283736 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1684171

New failing tests:
js/regress/function-with-eval.html

[Build Bot]

Created attachment 283743 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Created attachment 283833 [details]
the patch

[WebKit Commit Bot]

Attachment 283833 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5563:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5588:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:885:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:622:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:842:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 283836 [details]
the patch

[WebKit Commit Bot]

Attachment 283836 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5563:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5588:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:885:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:622:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:842:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 283843 [details]
performance

Big speed-up on micro benchmarks.  Interestingly, it's also a speed-up on a few old JSRegress tests.  That's reassuring.

[Saam Barati]

Comment on attachment 283836 [details]
the patch

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

r=me with questions and suggestions

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:458
> +            else if (m_graph.needsScopeRegister() && operand == m_codeBlock->scopeRegister())

A suggestion: To match the name needsFlushedThis(), you could call this "needsFlushedScope()"

> Source/JavaScriptCore/dfg/DFGClobberize.h:484
> +        read(AbstractHeap(Stack, virtualRegisterForArgument(0)));

Out of curiosity, why do we need this if we read(World)?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5582
> +        patchpoint->clobberLate(RegisterSet::volatileRegistersForJSCall());

Is it wrong to say this since the returnValueGPR should be in this set?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5603
> +                jit.addPtr(CCallHelpers::TrustedImm32(-static_cast<ptrdiff_t>(sizeof(CallerFrameAndPC))), CCallHelpers::stackPointerRegister, GPRInfo::regT1);

Are regT0 and regT1 guaranteed to be volatile on all platforms?

[Filip Pizlo]

(In reply to comment #25)
> Comment on attachment 283836 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283836&action=review
> 
> r=me with questions and suggestions
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:458
> > +            else if (m_graph.needsScopeRegister() && operand == m_codeBlock->scopeRegister())
> 
> A suggestion: To match the name needsFlushedThis(), you could call this
> "needsFlushedScope()"

I don't like that, since needsScopeRegister() also tells us if we need to do CodeBlock::setScopeRegister().  This is quite different from just flushing.

> 
> > Source/JavaScriptCore/dfg/DFGClobberize.h:484
> > +        read(AbstractHeap(Stack, virtualRegisterForArgument(0)));
> 
> Out of curiosity, why do we need this if we read(World)?

Yeah that's kinda dirty.  PreciseLocalClobberize turns read(World) into reads of locals that it thinks are escaping at that node.  It leaves read(Stack(specific location)) untouched.  So, having this extra read() has the effect of bypassing PreciseLocalClobberize's cleverness.  I could alternatively move this logic to PreciseLocalClobberize, but I'm not sure it matters that much.  It's kinda dirty either way.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5582
> > +        patchpoint->clobberLate(RegisterSet::volatileRegistersForJSCall());
> 
> Is it wrong to say this since the returnValueGPR should be in this set?

No, because when you pin the result to a specific register then this is the final register selection and Air will not care if that register interferes with anything else in the instruction.  It'll just trust you.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5603
> > +                jit.addPtr(CCallHelpers::TrustedImm32(-static_cast<ptrdiff_t>(sizeof(CallerFrameAndPC))), CCallHelpers::stackPointerRegister, GPRInfo::regT1);
> 
> Are regT0 and regT1 guaranteed to be volatile on all platforms?

Yes because they are temporary registers.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/203364

[Csaba Osztrogonác]

(In reply to comment #27)
> Landed in https://trac.webkit.org/changeset/203364

This one or r203365 made many tests crash on Apple Mac 32 bit bots:
- https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9824
- https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2948

( Last known good revision: r203363, first known bad revision: r203368 )

[Csaba Osztrogonác]

(In reply to comment #28)
> (In reply to comment #27)
> > Landed in https://trac.webkit.org/changeset/203364
> 
> This one or r203365 made many tests crash on Apple Mac 32 bit bots:
> -
> https://build.webkit.org/builders/Apple%20Yosemite%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/9824
> -
> https://build.webkit.org/builders/Apple%20El%20Capitan%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/2948
> 
> ( Last known good revision: r203363, first known bad revision: r203368 )

The backtrace seems to be related to eval, so probably this bug is the culprit.

[Csaba Osztrogonác]

bisecting on the bot is in progress:
- r203364: https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2956
- r203365: https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9834

[Csaba Osztrogonác]

(In reply to comment #30)
> bisecting on the bot is in progress:
> - r203364:
> https://build.webkit.org/builders/Apple%20El%20Capitan%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/2956
> - r203365:
> https://build.webkit.org/builders/Apple%20Yosemite%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/9834

Now it's sure, this bug is the culprit for the regression.