Bug 63858 – DFG JIT does not implement op_call

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

[Filip Pizlo]

The op_call opcode is used for function calls in JavaScript.  The DFG JIT does not implement this opcode, and hence cannot compile any functions that are not leaf functions.  The DFG JIT should implement this opcode, so that it can support a larger subset of JavaScript functions.

[Filip Pizlo]

Created attachment 99546 [details]
the patch

[Filip Pizlo]

Created attachment 99547 [details]
the patch (fix conflict)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/CodeLocation.h:124:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/assembler/CodeLocation.h:126:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/assembler/CodeLocation.h:128:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/bytecode/CodeBlock.h:383:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:436:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:400:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:380:  Declaration has space between type name and * in CodeBlock *callerCodeBlock  [whitespace/declaration] [3]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:393:  Missing spaces around =  [whitespace/operators] [4]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:476:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:995:  Declaration has space between type name and * in Instruction *putInstruction  [whitespace/declaration] [3]
Total errors found: 11 in 25 files


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

[Early Warning System Bot]

Comment on attachment 99547 [details]
the patch (fix conflict)

Attachment 99547 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8980113

[Collabora GTK+ EWS bot]

Comment on attachment 99547 [details]
the patch (fix conflict)

Attachment 99547 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8980114

[Filip Pizlo]

Created attachment 99548 [details]
the patch (fix)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGOperations.cpp:394:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:400:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 25 files


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

[Filip Pizlo]

Created attachment 99549 [details]
the patch (more fixes)

Accidentally left debug artifacts in the last patch.  Also, one file got accidentally omitted from the previous patch because of an unresolved svn conflict.  Both issues fixed in this patch.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGOperations.cpp:394:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:400:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 26 files


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

[Filip Pizlo]

Created attachment 99550 [details]
the patch (fix debug artifacts)

Removed two lingering debug printf's.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGOperations.cpp:394:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:400:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 26 files


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

[Mark Rowe (bdash)]

Comment on attachment 99550 [details]
the patch (fix debug artifacts)

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:36
> +#if ENABLE(DFG_JIT)
> +#include "DFGOperations.h"
> +#endif

The block of #include’s should be kept sorted, with any #if’s in separate paragraphs after the main unconditional block.

[Filip Pizlo]

Created attachment 99561 [details]
the path (address Mark's comment)

Thanks Mark!  This patch places the conditional include in a separate paragraph.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGOperations.cpp:394:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:400:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 26 files


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

[Gavin Barraclough]

Comment on attachment 99561 [details]
the path (address Mark's comment)

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

r- for a couple of minor nits, but on the whole looking really great.

> Source/JavaScriptCore/assembler/CodeLocation.h:122
> +class CodeLocationPossiblyNearCall : public CodeLocationCommon {

I'm not sure this extra type really adds any value.  If you want to change the type of the location held in the CodeBlock, you could just make it a CodeLocationLabel (after all, this is just a pointer into the instruction stream).  It seems highly unlikely this class will provide much value going forwards, since (1) we're likely to deprecate the old JIT, and (2) the new JIT is likely want to make near calls for JIT->JIT calls.  Still, I don't feel strongly about this - if it becomes redundant, it can be removed then.  So I'd recommend removing, but feel free to leave as is if you prefer.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:391
> +EncodedJSValue returnValueHack();

We don't like names like like with 'hack' in them - it doesn't convey any helpful information.  I'd suggest 'getHostCallReturnValue' would be a more descriptive function name here.

> Source/JavaScriptCore/interpreter/CallFrame.h:41
> +        JSValue calleeAsValue() const { return this[RegisterFile::Callee].jsValue(); }

I don't think this method should be needed; you should just be able to call 'callee()', and assign the result to a JSValue.  Furthermore, this may be incorrect - I'm not sure that we write the tag value for the callee on JVALUE32_64, we may just write the JSCell*, so reading the full 64-bit entry may result in an incorrect tag being read.  Did using 'callee()' cause any problems?

> Source/JavaScriptCore/runtime/JSFunction.h:40
> +    }

I don't think we normally doubly indent nested namespace declarations, this should probably all lose an indent level.

[Filip Pizlo]

> > Source/JavaScriptCore/assembler/CodeLocation.h:122
> > +class CodeLocationPossiblyNearCall : public CodeLocationCommon {
> 
> I'm not sure this extra type really adds any value.  If you want to change the type of the location held in the CodeBlock, you could just make it a CodeLocationLabel (after all, this is just a pointer into the instruction stream).  It seems highly unlikely this class will provide much value going forwards, since (1) we're likely to deprecate the old JIT, and (2) the new JIT is likely want to make near calls for JIT->JIT calls.  Still, I don't feel strongly about this - if it becomes redundant, it can be removed then.  So I'd recommend removing, but feel free to leave as is if you prefer.

Agree.

> 
> > Source/JavaScriptCore/dfg/DFGOperations.cpp:391
> > +EncodedJSValue returnValueHack();
> 
> We don't like names like like with 'hack' in them - it doesn't convey any helpful information.  I'd suggest 'getHostCallReturnValue' would be a more descriptive function name here.

Agree.

> 
> > Source/JavaScriptCore/interpreter/CallFrame.h:41
> > +        JSValue calleeAsValue() const { return this[RegisterFile::Callee].jsValue(); }
> 
> I don't think this method should be needed; you should just be able to call 'callee()', and assign the result to a JSValue.  Furthermore, this may be incorrect - I'm not sure that we write the tag value for the callee on JVALUE32_64, we may just write the JSCell*, so reading the full 64-bit entry may result in an incorrect tag being read.  Did using 'callee()' cause any problems?

I hadn't tried using callee().  The reason why I added this method is because the emitCall() path in DFGJITCodeGenerator.cpp never tests whether the target is a JSCell, and even the Speculative JIT does not speculate that it is a cell.  So at the point where we end up in DFGOperations, we could be dealing with an integer for all we know.

emitCall() currently always writes the JSValue - not the cell pointer - into the call frame.  Hence, logically, I wanted a method that returns the JSValue.  And on JSVALUE32_64, I imagine that we'd want to emit code like:

if tag != cell goto slow // unpatched compare/branch
if value != expected goto slow // patched compare, unpatched branch
perform fast call
goto done
slow:
write tag and value to call frame
call DFGOperations slow path
call *returnValueGPR
done:

The nice thing about this is that we have a common slow path for both the not-cell case and the not-expected-function case.

So, point is, my plan was that for both 32-bit and 64-bit platforms, I wanted the DFGOperations slow path to see both tag and payload.  Hence the calleeAsValue() method.  I certainly agree that this method makes no sense for the old JIT.  But in the DFG, the way I've currently implemented calls in this patch, it makes logical sense, to me at least.

Now, there's a separate question of whether this is optimal.  And whether or not it matters at all, since we're only targeting 64-bit.  For polymorphic calls where inline caching buys nothing, one could imagine that having a separate slow path for not-cell is better.

To make a long story short, I'm happy to make this change that you're recommending, though it would be a bit awkward given that callee() would be returning a JSCell even though we haven't validated that it's a JSCell.  I'm sure we could make that work on 64-bit.  Or I could change emitCall to have a separate slow path for not-cell.

So let me know if you buy the intuition for calleeAsValue(), or if you still recommend removing it.

> 
> > Source/JavaScriptCore/runtime/JSFunction.h:40
> > +    }
> 
> I don't think we normally doubly indent nested namespace declarations, this should probably all lose an indent level.

Got it.  Will change.

[Gavin Barraclough]

My bad, my concern re callee() was misplaced.  I thought that the 'function()' method called in the existing callee() was only reading 32-bits on JSVALUE32_64 - in which case it would be bad if we were relying on the aliasing within the Register struct - but this has changed, it also is performing the full 64-bit access internally.  I'm happy with your change.

[Filip Pizlo]

Created attachment 99739 [details]
the patch (fix review)

[Filip Pizlo]

Created attachment 99748 [details]
the patch (fix merge conflicts)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGOperations.cpp:404:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:410:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 25 files


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

[WebKit Review Bot]

Comment on attachment 99748 [details]
the patch (fix merge conflicts)

Rejecting attachment 99748 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 1

Last 500 characters of output:
030e685994e7463aaab925c058def0f05c4737ab
r90412 = afaca2bd5d09b84d41d2590bbdc855d9cbfd7aaf
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/8987481

[Filip Pizlo]

Created attachment 99764 [details]
the patch (fix more merge conflicts)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGOperations.cpp:404:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:410:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 25 files


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

[WebKit Review Bot]

Comment on attachment 99764 [details]
the patch (fix more merge conflicts)

Clearing flags on attachment: 99764

Committed r90423: <http://trac.webkit.org/changeset/90423>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.