[webkit-reviews] review denied: [Bug 56284] Add a dataflow intermediate representation for use in JIT generation. : [Attachment 85637] More style issues (remaining issues not to be fixed; current formatting is clearer).

Sun Mar 13 21:51:32 PDT 2011

Geoffrey Garen <ggaren at apple.com> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 56284: Add a dataflow intermediate representation for use in JIT
generation.
https://bugs.webkit.org/show_bug.cgi?id=56284

Attachment 85637: More style issues (remaining issues not to be fixed; current
formatting is clearer).
https://bugs.webkit.org/attachment.cgi?id=85637&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.