Bug 63858

Summary: DFG JIT does not implement op_call
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, gns, gustavo.noronha, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 63980    
Attachments:
Description Flags
the patch
none
the patch (fix conflict)
webkit-ews: commit-queue-
the patch (fix)
none
the patch (more fixes)
none
the patch (fix debug artifacts)
none
the path (address Mark's comment)
barraclough: review-
the patch (fix review)
none
the patch (fix merge conflicts)
barraclough: review+, webkit.review.bot: commit-queue-
the patch (fix more merge conflicts) none

Description Filip Pizlo 2011-07-01 23:25:38 PDT
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.
Comment 1 Filip Pizlo 2011-07-01 23:34:19 PDT
Created attachment 99546 [details]
the patch
Comment 2 Filip Pizlo 2011-07-01 23:52:34 PDT
Created attachment 99547 [details]
the patch (fix conflict)
Comment 3 WebKit Review Bot 2011-07-01 23:54:24 PDT
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.
Comment 4 Early Warning System Bot 2011-07-02 00:04:31 PDT
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
Comment 5 Collabora GTK+ EWS bot 2011-07-02 00:11:00 PDT
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
Comment 6 Filip Pizlo 2011-07-02 00:36:46 PDT
Created attachment 99548 [details]
the patch (fix)
Comment 7 WebKit Review Bot 2011-07-02 00:39:51 PDT
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.
Comment 8 Filip Pizlo 2011-07-02 01:17:19 PDT
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.
Comment 9 WebKit Review Bot 2011-07-02 01:19:20 PDT
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.
Comment 10 Filip Pizlo 2011-07-02 01:37:45 PDT
Created attachment 99550 [details]
the patch (fix debug artifacts)

Removed two lingering debug printf's.
Comment 11 WebKit Review Bot 2011-07-02 01:40:25 PDT
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.
Comment 12 Mark Rowe (bdash) 2011-07-02 14:14:16 PDT
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.
Comment 13 Filip Pizlo 2011-07-02 14:51:23 PDT
Created attachment 99561 [details]
the path (address Mark's comment)

Thanks Mark!  This patch places the conditional include in a separate paragraph.
Comment 14 WebKit Review Bot 2011-07-02 14:53:41 PDT
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.
Comment 15 Gavin Barraclough 2011-07-03 18:36:59 PDT
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.
Comment 16 Filip Pizlo 2011-07-03 23:30:21 PDT
> > 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.
Comment 17 Gavin Barraclough 2011-07-04 18:45:45 PDT
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.
Comment 18 Filip Pizlo 2011-07-05 13:51:14 PDT
Created attachment 99739 [details]
the patch (fix review)
Comment 19 Filip Pizlo 2011-07-05 14:48:33 PDT
Created attachment 99748 [details]
the patch (fix merge conflicts)
Comment 20 WebKit Review Bot 2011-07-05 14:50:31 PDT
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.
Comment 21 WebKit Review Bot 2011-07-05 16:10:53 PDT
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
Comment 22 Filip Pizlo 2011-07-05 17:04:53 PDT
Created attachment 99764 [details]
the patch (fix more merge conflicts)
Comment 23 WebKit Review Bot 2011-07-05 17:08:04 PDT
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.
Comment 24 WebKit Review Bot 2011-07-05 17:57:03 PDT
Comment on attachment 99764 [details]
the patch (fix more merge conflicts)

Clearing flags on attachment: 99764

Committed r90423: <http://trac.webkit.org/changeset/90423>
Comment 25 WebKit Review Bot 2011-07-05 17:57:10 PDT
All reviewed patches have been landed.  Closing bug.