Bug 142072 – [JSC] Use the way number constants are written to help type speculation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] Use the way number constants are written to help type speculation

[Benjamin Poulain]

Created attachment 247491 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 24 files


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

[Geoffrey Garen]

Comment on attachment 247491 [details]
Patch

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

Looks reasonable to me. Probably useful to get Phil to review too.

> Source/JavaScriptCore/bytecode/CodeBlock.h:638
>          m_constantRegisters.last().set(m_globalObject->vm(), m_ownerExecutable.get(), v);
> +        m_constantsSourceCodeRepresentation.append(SourceCodeRepresentation::Other);

Is there one entry in m_constantsSourceCodeRepresentation for each entry in m_constantRegisters? If so, can we just make m_constantRegisters a vector of pairs, or some kind of struct, so we don't have to worry about keeping things in sync?

[Benjamin Poulain]

Comment on attachment 247491 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/CodeBlock.h:638
>> +        m_constantsSourceCodeRepresentation.append(SourceCodeRepresentation::Other);
> 
> Is there one entry in m_constantsSourceCodeRepresentation for each entry in m_constantRegisters? If so, can we just make m_constantRegisters a vector of pairs, or some kind of struct, so we don't have to worry about keeping things in sync?

The reason for keeping them separate is GC. For example in CodeBlock, CodeBlock::stronglyVisitStrongReferences() visit the whole memory buffer of m_constantRegisters.

We could still mark them one by one but I figured we should not pay any GC cost for this feature.

[Filip Pizlo]

Comment on attachment 247491 [details]
Patch

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

I'd like to see a solution that doesn't add fields or constructor overloads to Node, but that uses NodeFlags.

>> Source/JavaScriptCore/bytecode/CodeBlock.h:638
>> +        m_constantsSourceCodeRepresentation.append(SourceCodeRepresentation::Other);
> 
> Is there one entry in m_constantsSourceCodeRepresentation for each entry in m_constantRegisters? If so, can we just make m_constantRegisters a vector of pairs, or some kind of struct, so we don't have to worry about keeping things in sync?

Two downsides of what you propose.

First, we wouldn't be able to BaseIndex into the m_constantRegisters.  LLInt does that a lot.  BaseIndex addressing maxes out at a shift amount of 3, i.e. an array element size of 8.  So if you made it an array of pairs, you'd have to use an explicit shift to get the offset into constantRegisters from the index.

Second, you'd use more memory.  The pair structure would become aligned to the max native alignment of its fields, which is 8.  So that representation would become an 8 byte element.  Using two arrays means that the representation can be arbitrarily compact (could even be a bit in the future).

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:674
> +        Node* result = m_graph.addNode(SpecNone, NodeOrigin(currentCodeOrigin()), info, sourceCodeRepresentation);
> +        m_currentBlock->append(result);
> +        return result;

Why doesn't this do addToGraph()?  Why can't we put the SourceCodeRepresentation into the NodeFlags of JSConstant?

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1056
> +    m_codeBlock->constantsSourceCodeRepresentation().resize(0);

Probably my feedback will involve a lot of comments to the effect of: why can't we put the SourceCodeRepresentation into the NodeFlags of JSConstant?

> Source/JavaScriptCore/dfg/DFGNode.h:285
> +    Node(NodeOrigin nodeOrigin, OpInfo imm, SourceCodeRepresentation sourceCodeRepresentation)
> +        : origin(nodeOrigin)
> +        , children()
> +        , m_constantSourceRepresentation(static_cast<unsigned>(sourceCodeRepresentation))
> +        , m_virtualRegister(VirtualRegister())
> +        , m_refCount(1)
> +        , m_prediction(SpecNone)
> +        , m_opInfo(imm.m_value)
> +        , m_opInfo2(0)
> +        , replacement(nullptr)
> +        , owner(nullptr)
> +    {
> +        setOpAndDefaultFlags(JSConstant);
> +        m_constantSourceRepresentation = static_cast<unsigned>(sourceCodeRepresentation);
> +        ASSERT(!(m_flags & NodeHasVarArgs));
> +    }
> +

Seems like overkill to add a new Node overload for this.  You wouldn't have to if it was just a NodeFlags.

> Source/JavaScriptCore/dfg/DFGNode.h:1933
> +    unsigned m_constantSourceRepresentation: 2;

So, this probably increases the size of Node (10 + 22 + 2 = 34, previously it was 32 - so previously it used one 32-bit int and now it uses more space).  Also, we generally avoid adding new fields to Node and instead just add a flag in NodeFlags.

[Filip Pizlo]

Comment on attachment 247491 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:145
> -            SpeculatedType type = speculationFromValue(node->asJSValue());
> +            SpeculatedType type = speculationFromValue(node->asJSValue(), node->sourceCodeRepresentation());

Why did you do this, instead of just having node->sourceCodeRepresentation deactivate the Int52 hack below?

[Filip Pizlo]

I think that in the DFG the best way to do this is to use DoubleConstant in ByteCodeParser for when the source code representation is Double, and then make sure that PredictionPropagation understands DoubleConstant.  Then you don't have to change the IR so much, and it keeps things consistent - DoubleConstant already has the meaning you want but happens to not get used by the PredictionPropagation phase.

[Benjamin Poulain]

Created attachment 247564 [details]
Patch

[Benjamin Poulain]

Created attachment 247565 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 24 files


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

[Filip Pizlo]

Comment on attachment 247565 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:264
> +                OpInfo opInfo(m_graph.freezeStrong(value));

I guess that the value is guaranteed to be isDouble() if we have SourceCodeRepresentation::Double.  Can we assert it?

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:156
> +            double number = node->asNumber();
> +            if (number != number)
> +                changed |= setPrediction(SpecDoublePureNaN);
> +            else
> +                changed |= setPrediction(SpecFullRealNumber);

Why can't this be speculationFromValue()?

> Source/JavaScriptCore/dfg/DFGValidate.cpp:558
> +    void validateEdgeWithDoubleResultIfNecessary(Node* node, Edge edge)
> +    {
> +        if (!edge->hasDoubleResult())
> +            return;
> +
> +        // The only case were an edge can have a DoubleResult without having a number-type useKind()
> +        // for constants that appear as double in the source code (e.g. 42.0e2).
> +        if (edge->op() == DoubleConstant)
> +            VALIDATE((node, edge), edge.useKind() == UntypedUse || edge.useKind() == DoubleRepUse || edge.useKind() == DoubleRepRealUse || edge.useKind() == DoubleRepMachineIntUse);
> +        else
> +            VALIDATE((node, edge), edge.useKind() == DoubleRepUse || edge.useKind() == DoubleRepRealUse || edge.useKind() == DoubleRepMachineIntUse);
> +    }

I'm super uncomfortable with this change.  It's undesirable because after FixupPhase, we *will* have DoubleRepUse (or its friends) for every DoubleResult node, and we must assert that this is true - otherwise for example we'll crash while lowering to LLVM, or while emitting machine code in DFGSpeculativeJIT.

There are probably three OK solutions:

- Make the representation validation contingent upon FixupPhase having run.  It's OK to have a m_fixupDidRun bool in Graph, or better yet make it an enum.  We have a bunch of such enums in Graph.

- Have ByteCodeParser plant a DoubleRepUse on DoubleConstant.  This would be easy; you could probably make it work by changing code like this in ByteCodeParser:

    Node* addToGraph(NodeType op, Node* child1 = 0, Node* child2 = 0, Node* child3 = 0)
    {
        Node* result = m_graph.addNode(
            SpecNone, op, currentNodeOrigin(), Edge(child1), Edge(child2),
            Edge(child3));

to instead do:

        Node* result = m_graph.addNode(
            SpecNone, op, currentNodeOrigin(), child1->defaultEdge(), child2->defaultEdge(),
            child3->defaultEdge());

- Go back to using JSConstant and use a NodeFlag to indicate representation.

I think I like the second one the best if it "just works", otherwise the third one feels most right.

[Filip Pizlo]

Comment on attachment 247565 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:113
> +        * bytecode/SpeculatedType.cpp:
> +        (JSC::speculationFromValue):
> +        This is where the magic comes in. When integer values were represented as double,
> +        speculate full number instead of Int32/Int52.

You no longer have this change. :-)

[Benjamin Poulain]

Created attachment 247575 [details]
Patch

[Benjamin Poulain]

Created attachment 247578 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 25 files


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

[Benjamin Poulain]

Comment on attachment 247578 [details]
Patch

Clearing flags on attachment: 247578

Committed r180813: <http://trac.webkit.org/changeset/180813>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.