Bug 56284 – Add a dataflow intermediate representation for use in JIT generation.

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

[Gavin Barraclough]

The JSC JIT presently generates code directly from the bytecode used by the interpreter.  This is not an optimal intermediate representation for JIT code generation, since it does capture liveness information of values, and provides little opportunity to perform any static analysis for even primitive types.  The JIT currently generates two code paths, a fast path handling common cases, and a slower path handling less common operand types.  However the slow path jumps back into the fast path, meaning that information arising from the earlier type checks cannot be propagated to later operations.

This patch adds:
    * a dataflow intermediate representation capable of describing a single basic block of operations,
    * a mechanism to convert a simple, single-block bytecode functions to the new IR,
    * and a JIT code generator capable of generating code from this representation.

The JIT generates two code paths, with the slower path not reentering the fast path mid-block, allowing speculative optimizations to be made on the hot path, with type information arising from these speculative decisions able to be propagated through the dataflow.  Code generation of both speculative and non-speculative paths exploits the type and liveness information represented in the dataflow graph to attempt to avoid redundant boxing and type-checking of values, and to remove unnecessary spills of temporary values to the RegisterFile.

The dataflow JIT currently can only support a subset of bytecode operations, limited to arithmetic, bit-ops, and basic property access.  Functions that cannot be compiled by the dataflow JIT will be run using the existing JIT.  The coverage of the dataflow JIT will be expanded to include, control-flow, function calls, and then the long-tail of remaining bytecode instructions.  The JIT presently only support JSVALUE64, and as a consequence of this only supports x86-64.

The status of the dataflow JIT is currently work-in-progress.  Limitations of the present JIT code generation may cause performance regressions, particularly:
    * the policy to only generate arithmetic code on the speculative path using integer instructions, never using floating point.
    * the policy to only generate arithmetic code on the non-speculative path using floating point instructions, never using integer.
    * always generating JSValue adds on the non-speculative path as a call out to a C function, never handling this in JIT code.
    * always assuming by-Value property accesses on the speculative path to be array accesses.
    * generating all by-Value property accesses from the non-speculative path as a call out to a C function.
    * generating all by-Indentifer property accesses as a call out to a C function.
Due to these regressions, the code is landed in a state where it is disabled in most cases by the ENABLE_DFG_JIT_RESTRICTIONS guard in Platform.h.  As these regressions are addressed, the JIT will be allowed to trigger in more cases.

[Gavin Barraclough]

Created attachment 85634 [details]
The patch, no performance impact.

[WebKit Review Bot]

Attachment 85634 [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

Last 3072 characters of output:
:66:  The parameter name "nodeIndex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:67:  The parameter name "nodeIndex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:68:  The parameter name "nodeIndex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:460:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:461:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:488:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:489:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:562:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:664:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.h:191:  The parameter name "operand" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.h:192:  The parameter name "operand" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:64:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:196:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:132:  The parameter name "nodeIndex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:133:  The parameter name "nodeIndex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:134:  The parameter name "nodeIndex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:159:  The parameter name "nodeIndex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:792:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:871:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 72 in 20 files


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

[Gavin Barraclough]

Created attachment 85635 [details]
Fix style issues.

[WebKit Review Bot]

Attachment 85635 [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

Last 3072 characters of output:
e/operators] [3]
Source/JavaScriptCore/dfg/DFGGraph.h:112:  Missing spaces around |  [whitespace/operators] [3]
Source/JavaScriptCore/dfg/DFGGraph.h:117:  Missing spaces around |  [whitespace/operators] [3]
Source/JavaScriptCore/dfg/DFGGraph.h:119:  Missing spaces around |  [whitespace/operators] [3]
Source/JavaScriptCore/dfg/DFGGraph.h:121:  Missing spaces around |  [whitespace/operators] [3]
Source/JavaScriptCore/dfg/DFGGraph.h:124:  Missing spaces around |  [whitespace/operators] [3]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:47:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:49:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:51:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGGraph.cpp:62:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:57:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:57:  SPILL_ORDER_CONSTANT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:58:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:58:  SPILL_ORDER_SPILLED is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:59:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:59:  SPILL_ORDER_ARGUMENT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:60:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:60:  SPILL_ORDER_JS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:61:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:61:  SPILL_ORDER_CELL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:62:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:62:  SPILL_ORDER_INTEGER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:63:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:63:  SPILL_ORDER_DOUBLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 41 in 20 files


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

[Early Warning System Bot]

Attachment 85634 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8177124

[Early Warning System Bot]

Attachment 85635 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8175130

[Build Bot]

Attachment 85634 [details] did not build on win:
Build output: http://queues.webkit.org/results/8176137

[Gavin Barraclough]

Created attachment 85637 [details]
More style issues (remaining issues not to be fixed; current formatting is clearer).

[WebKit Review Bot]

Attachment 85637 [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/DFGRegisterBank.h:47:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:49:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:51:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGGraph.cpp:62:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:57:  SPILL_ORDER_CONSTANT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:58:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:58:  SPILL_ORDER_SPILLED is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:59:  SPILL_ORDER_ARGUMENT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:60:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:60:  SPILL_ORDER_JS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:61:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:61:  SPILL_ORDER_CELL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:62:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:62:  SPILL_ORDER_INTEGER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:63:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:63:  SPILL_ORDER_DOUBLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 16 in 20 files


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

[Build Bot]

Attachment 85635 [details] did not build on win:
Build output: http://queues.webkit.org/results/8177128

[Mark Rowe (bdash)]

Comment on attachment 85637 [details]
More style issues (remaining issues not to be fixed; current formatting is clearer).

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

> Source/JavaScriptCore/ChangeLog:9
> +        This is not an optimal intermediate representation for JIT code generation, since it does
> +        capture liveness information of values, and provides little opportunity to perform any

I suspect this should say “does not capture liveness information”.

[Geoffrey Garen]

Comment on attachment 85637 [details]
More style issues (remaining issues not to be fixed; current formatting is clearer).

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

What is the performance impact of this patch?

I think there's enough to chew on here for another round of review.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1997
> +				86EC9DBF1328DF82002B2AD7 /* DFGOperations.cpp */,
> +				86EC9DC01328DF82002B2AD7 /* DFGOperations.h */,

Kind of a shame that our two JITs use different names for their C helper functions.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:38
> +class DataFlowParser {

Class name should match file name (module "DFG").

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:107
> +        // We have not yet seen a definition for this value in this block.
> +        // For now, since we are only generating single block functions,
> +        // this value must be undefined.
> +        return constantUndefined();

This is pretty weird. Would be worth giving an example of when this can happen to show why this is correct behavior. Maybe an ASSERT, too, since this won't stay true forever.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:159
> +        // Check if the operand is already an int32 - if so, nothing to do!

This comment doesn't add anything over the code.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:196
> +        // Check if the operand is already a double - if so, nothing to do!

This comment doesn't add anything over the code.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:231
> +        return m_constantRecords[constant].asInt32 = resultIndex;

Please separate assignment from return.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:240
> +        return m_constantRecords[constant].asNumeric = resultIndex;

Please separate assignment from return.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:249
> +        return m_constantRecords[constant].asJSValue = resultIndex;

Please separate assignment from return.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:316
> +    }

Seems like we could simplify things a lot if we just required CodeBlock always to hold undefined as a constant.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:342
> +    }

Ditto. (Slightly more controversial here, I guess.)

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:347
> +    NodeIndex addToGraph(NodeType op, NodeIndex child1 = (NodeIndex)0, NodeIndex child2 = (NodeIndex)0, NodeIndex child3 = (NodeIndex)0)

I see magic "(NodeIndex)0" in a lot of places. I think it would be better to have a typedef for "NullNodeIndex" or "InvalidNodeIndex".

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:388
> +    // CodeBlock's constant pool. These varibles hold are initialized to

Typos: "varibles". "hold are".

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:413
> +    Vector <NodeIndex, 32> m_registers;

Would be nice to standardize terminology with CodeBlock, which calls these "parameters" and "callee registers". I don't really care which one wins, as long as they agree.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:418
> +    // These maps are used to unique ToNumber and ToInt32 operations.
> +    typedef HashMap<NodeIndex, NodeIndex> UnaryOpCSEMap;
> +    UnaryOpCSEMap m_int32ToNumberNodes;
> +    UnaryOpCSEMap m_numberToInt32Nodes;

I think you can cut "CSE" out of the name to make it easier to read. Yes, you use these maps for CSE, but in the end they're just maps.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:433
> +    bool noArith = true;

I think we can afford the "metic" here, no?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:660
> +            if (candidateAliasGetByVal) {
> +                Node& possibleAlias = m_graph[candidateAliasGetByVal];
> +                ASSERT(possibleAlias.op == GetByVal || possibleAlias.op == GetByValAlias);
> +                // This check ensures the accesses alias, provided that the subscript is an
> +                // integer index (this is good enough; the speculative path will only generate
> +                // optimized accesses to handle integer subscripts).
> +                if (possibleAlias.child1 == base && equalIgnoringLaterNumericConversion(possibleAlias.child2, property)) {
> +                    NodeIndex aliasedGetByVal = possibleAlias.op == GetByValAlias ? possibleAlias.child3 : candidateAliasGetByVal;
> +                    getByVal = addToGraph(GetByValAlias, base, property, aliasedGetByVal);
> +                }
> +            }

I think it would be good to abstract the lookup for an aliasing property access behind a property aliasing helper class. Right now, the code for maintaining this data is all over the place, and somewhat duplicated.

I'm sure we'll want to change this optimization to make it more robust in the future, and putting it in its own class will make that easier.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:759
> +class ScoreBoard {

Can this class go in its own file?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:808
> +        // child 0 means no child!
> +        if (!child)
> +            return;

NullNodeIndex or InvalidNodeIndex, please.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:848
> +    // Asign VirtualRegisters.

Typo: "Asign".

> Source/JavaScriptCore/dfg/DFGGenerationInfo.h:231
> +    // The index of the node whose result is stored in this virtual register.
> +    // FIXME: Can we remove this? - this is currently only used 

Seems like you trailed off here.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:110
> +    // If the node has a first child, then ref it!

Comments doesn't add anything over the code.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:117
> +    // If the node has a second child, then ref it!

Comments doesn't add anything over the code.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:124
> +    // If the node has a third child, then ref it!

Comments doesn't add anything over the code.

> Source/JavaScriptCore/dfg/DFGGraph.h:79
> +    /* The first entry in the graph array is a sentinel value. */\
> +    /* This means no other node should be referencing element 0,  */\
> +    /* and as such the NodeIndex value 0 can be used to mean 'none'. */\
> +    macro(BeginMarker, 0) \

Can we just use -1 instead, as with VirtualRegister? That would allow you to get rid of BeginMarker.

> Source/JavaScriptCore/dfg/DFGGraph.h:156
> +struct Node {

Can this class go in its own file?

> Source/JavaScriptCore/dfg/DFGGraph.h:320
> +    // When a node's refCount goes from 0 to 1, it must (logially) recursively ref all of its children.

Typo: logially.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:73
> +        // Assume DataFormatJSInteger! - we'll guard this with a jitAssert below!

A better comment would explain how we know we've eliminated all possibilities other than integer.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:85
> +        CRASH();

Why CRASH() instead of ASSERT?

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:165
> +    case DataFormatNone:
> +        CRASH();
> +
> +    case DataFormatCell:
> +    case DataFormatJSCell:
> +        CRASH();

Why CRASH() instead of ASSERT?

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:213
> +    // We must now be at the end of both iterators!

This comment doesn't add anything over the code.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:219
> +void JITCompiler::compileFunction(JITCode& entry, MacroAssemblerCodePtr& entryWithArityCheck)

A lot of the FIXMEs in this function would be better as bug reports. They don't pertain to this function in particular -- they pertain to the overall design of the engine.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:352
> +    //  Are we generating exception handling info? - we only do so if the user is debugging
> +    // (and may want line number info), or if this function contains exception handler.
> +    if (m_codeBlock->needsCallReturnIndices()) {

This comment belongs in the header alongside needsCallReturnIndices(), not here.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:367
> +    // Done! - return the two entry points to the code.

This comment doesn't add anything over the code.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.h:84
> +    // These methods are used when generating 'unexpected'
> +    // calls out from JIT code to C++ helper routines -
> +    // they spill all live values to the appropriate

Note that any call from the JIT to a C helper must spill and fill all register values, for GC correctness.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:104
> +    // FIXME: if toString threw an exception, we shouldn't get, in case this triggers a getter?
> +    // Explicitly check for properties already being strings to avoid the extra exception check in the common case?
> +    // Oh! no - this is probably okay. If toString throws, ident is probably null, and objects can't have null properties,
> +    // so the get will fail? Should add a test case, check the value thrown from toString is passed through correctly, etc.

Probably better not to have a conversation with yourself in source code. I'd suggest a bug report instead.

[Gavin Barraclough]

> I suspect this should say “does not capture liveness information”.

Indeed it should.

[Gavin Barraclough]

All the issues that were a simple fix, I haven't comment on.

> What is the performance impact of this patch?

None (according to SunSpider).

> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1997
> > +				86EC9DBF1328DF82002B2AD7 /* DFGOperations.cpp */,
> > +				86EC9DC01328DF82002B2AD7 /* DFGOperations.h */,
> 
> Kind of a shame that our two JITs use different names for their C helper functions.

I've grown to dislike the old term 'stub'.  'Stub' is pretty meaningless, and seems to imply something small, whereas the helper functions called out to form JIT code could be arbitrarily large (and some operations are quite complex).  I picked 'operations' to match runime/Operations.h, which implements many similar routines used by the interpreter (and also in the implementation of many JIT stub functions).  I could switch to 'Stubs', but I'd prefer to stick with 'Operations', unless you have any alternative suggestions?

> > +        // this value must be undefined.
> > +        return constantUndefined();
> This is pretty weird. Would be worth giving an example of when this can happen to show why this is correct behavior. Maybe an ASSERT, too, since this won't stay true forever.

Commented, not sure how to go about defending this with an ASSERT, not sure this is a major concern - it will be pretty clear as we add flow control that values defined by prior blocks shouldn't read as undefined. :-)

> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:316
> Seems like we could simplify things a lot if we just required CodeBlock always to hold undefined as a constant.
> Ditto. (Slightly more controversial here, I guess.)

Agreed, I'd rather not change byte compilation in this patch, and I don't think the code is *too* squirley here, so I'm inclined to leave as is, and maybe change in the future to guarantee undefined in the CodeBlock.  (Or course, in the future future, this may not really be an issue, since we may just switch to going straight from AST to DFG).  What do you think?

> I see magic "(NodeIndex)0" in a lot of places. I think it would be better to have a typedef for "NullNodeIndex" or "InvalidNodeIndex".

I think I like the terse "NoNode", which is what it means.  Sound okay to you?

> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:413
> > +    Vector <NodeIndex, 32> m_registers;
> Would be nice to standardize terminology with CodeBlock, which calls these "parameters" and "callee registers". I don't really care which one wins, as long as they agree.

Agreed.  Given that JavaScript has the Arguments object, I think parameters loses.  I won't rename in CodeBlock as a part of this patch.  (m_registers changed to m_calleeRegisters)

> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:660
> > +            if (candidateAliasGetByVal) {
> > +                Node& possibleAlias = m_graph[candidateAliasGetByVal];
> > +                ASSERT(possibleAlias.op == GetByVal || possibleAlias.op == GetByValAlias);
> > +                // This check ensures the accesses alias, provided that the subscript is an
> > +                // integer index (this is good enough; the speculative path will only generate
> > +                // optimized accesses to handle integer subscripts).
> > +                if (possibleAlias.child1 == base && equalIgnoringLaterNumericConversion(possibleAlias.child2, property)) {
> > +                    NodeIndex aliasedGetByVal = possibleAlias.op == GetByValAlias ? possibleAlias.child3 : candidateAliasGetByVal;
> > +                    getByVal = addToGraph(GetByValAlias, base, property, aliasedGetByVal);
> > +                }
> > +            }
> I think it would be good to abstract the lookup for an aliasing property access behind a property aliasing helper class. Right now, the code for maintaining this data is all over the place, and somewhat duplicated.
> I'm sure we'll want to change this optimization to make it more robust in the future, and putting it in its own class will make that easier.

Will do.  I'll put up another patch without this change, then follow up with another with.

> > +    macro(BeginMarker, 0) \
> Can we just use -1 instead, as with VirtualRegister? That would allow you to get rid of BeginMarker.
This would not be a trivial change, and I think would be less readable and more error-prone (it seems highly intuitive to me to check for a Node's children with "if (node.child1)", etc, rather than having to know that you need to be checking for a magic value or special constant).  These are also checked frequently in some high traffic routines, and checking for zero is typically faster.  Do you mind if I keep it this way?
(This approach was not as practical for VirtualRegister because the byte compiler starts numbering locals at 0, but I would like to change VirtualRegister to match).

> Note that any call from the JIT to a C helper must spill and fill all register values, for GC correctness.
They should all do so! - all expected calls use flushRegisters(), implicit conversions call silentSpill()/silentFill().  No values that were in registers before a call to C code will be used after the call.  All values will have been reloaded from the RegisterFile.  So long as GC is updating the RegisterFile correctly, we should be fine.

[Gavin Barraclough]

Created attachment 85651 [details]
With geoff's comments fixed (bar abstracting out the alias mechanism).

[WebKit Review Bot]

Attachment 85651 [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/DFGGraph.cpp:62:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:47:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:49:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:51:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:57:  SPILL_ORDER_CONSTANT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:58:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:58:  SPILL_ORDER_SPILLED is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:59:  SPILL_ORDER_ARGUMENT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:60:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:60:  SPILL_ORDER_JS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:61:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:61:  SPILL_ORDER_CELL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:62:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:62:  SPILL_ORDER_INTEGER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:63:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:63:  SPILL_ORDER_DOUBLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 17 in 23 files


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

[Oliver Hunt]

Comment on attachment 85651 [details]
With geoff's comments fixed (bar abstracting out the alias mechanism).

The SPILL_ORDER constants should be changed to an enum type with SpillOrderConstant, SpillOrderSpilled, etc -- afaict retain() is only given these values so it should be restricted by the type system.

[Oliver Hunt]

Comment on attachment 85651 [details]
With geoff's comments fixed (bar abstracting out the alias mechanism).

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

r- due to various style issues

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:66
> +            m_jit.loadPtr(MacroAssembler::Address(JITCompiler::callFrameRegister, (argument - (m_jit.codeBlock()->m_numParameters + RegisterFile::CallFrameHeaderSize)) * sizeof(Register)), reg);

This should use addressFor(argument - (m_jit.codeBlock()->m_numParameters + RegisterFile::CallFrameHeaderSize)) rather than explicitly calling the Address constructor.  It's much easier to read.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:70
> +            m_jit.loadPtr(MacroAssembler::Address(JITCompiler::callFrameRegister, virtualRegister * sizeof(Register)), reg);

addressFor(virtualRegister)

Using addressFor(), etc makes it much harder to screwup in the face of different byte orders, etc.   eg. int loads/stores need to go to the right part of the register.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:157
> +            m_jit.loadPtr(MacroAssembler::Address(JITCompiler::callFrameRegister, virtualRegister * sizeof(Register)), reg);

I strongly recommend just going through this patch and looking for any use of Address() that is based of of callFrameRegister and swith to addressFor

[Gavin Barraclough]

Created attachment 85732 [details]
Separate out DFG::AliasTracker, make NoNode==UINT_MAX, remove the BeginMarker sentinel, add addressFor/addressForGlobalVar/addressForArgument calls.

[Gavin Barraclough]

Ooops, patch has #define ENABLE_DFG_JIT_RESTRICTIONS 0 due to testing, will check this is set to 1 before landing!

[WebKit Review Bot]

Attachment 85732 [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/DFGGraph.cpp:60:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:47:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:49:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRegisterBank.h:51:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:60:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:62:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:63:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:64:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:65:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 24 files


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

[Geoffrey Garen]

Comment on attachment 85732 [details]
Separate out DFG::AliasTracker, make NoNode==UINT_MAX, remove the BeginMarker sentinel, add addressFor/addressForGlobalVar/addressForArgument calls.

r=me

[Gavin Barraclough]

fixed in r81079.