Bug 196639 – Clean up Int52 code and some bugs in it

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 366792 [details]
WIP

It begins.

[Saam Barati]

Things that look suspect or are wrong.


(1):
1: JSConstant(10.0) => m_type is SpecAnyIntAsDouble
2: Int52Rep(@1) => m_type is SpecInt52Only
3: ValueRep(@2) => m_type is SpecAnyIntAsDouble

However, when we run the above program, @3 will produce a boxed int32.

(2):
inline bool isAnyIntSpeculation(SpeculatedType value)
{
    return !!value && (value & SpecAnyInt) == value;
}

This looks suspect because we sometimes use this to say if we're going to do Int52 speculation. There are cases where we really want to check (value & (SpecInt32Only | SpecInt52Only | SpecAnyIntAsDouble)) since we're dealing with predictions generated from bytecode. Some places manually handle their result possibly being Int52 and fill in the necessary speculated type bits. But other places just use their heap predictions, which will never have the SpecInt52Only bit set...

(3):
There is code like this for add in fixup:
    else if (m_graph.addShouldSpeculateAnyInt(node))
        changed |= mergePrediction(SpecInt52Only);

However, my understanding is that Int52Only means i52 out of i32 range.


(4):
        case JSConstant: {
            SpeculatedType type = speculationFromValue(m_currentNode->asJSValue());
            if (type == SpecAnyIntAsDouble && enableInt52())
                type = SpecInt52Only;
            setPrediction(type);
            break;
        }

This code also looks suspect for reasons in (3).

[Saam Barati]

Most places that use (2) are doing the right thing. But some are probably being too conservative.

[Saam Barati]

Created attachment 366863 [details]
WIP

I'm just moving things to a segregated Int52 lattice

[Saam Barati]

Created attachment 366869 [details]
WIP

Might be done in spirit. Still gonna read through the code one more time. Need to revert some release asserts back to debug asserts and clean up some commented out code.

[Saam Barati]

(In reply to Saam Barati from comment #5)
> Created attachment 366869 [details]
> WIP
> 
> Might be done in spirit. Still gonna read through the code one more time.
> Need to revert some release asserts back to debug asserts and clean up some
> commented out code.

Might be a good time for anyone interested in this to take a look at the code to see what they think.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/dfg/DFGAbstractValue.h:425:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractValue.h:428:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:738:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:443:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:458:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:459:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:460:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/SpeculatedType.cpp:254:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 8 in 16 files


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

[Saam Barati]

<rdar://problem/49515757>

[Saam Barati]

Created attachment 366907 [details]
patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/bytecode/SpeculatedType.cpp:255:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


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

[Yusuke Suzuki]

Comment on attachment 366907 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:16
> +        However, this got confusing because we reused SpecInt32Only both for JSValue
> +        representations and Int52 representations. This actually lead to some bugs.

I like this idea, it simplifies shouldSpeculateInt52 flow too, and now we do not need to bother whether we should include SpecInt32Only in shouldSpeculateInt52. (This should be handled by shouldSpeculateInt32 thing. shouldSpeculateInt52 is now used to propagate Int52 format flow).

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:768
>                      break;

`shouldCheckOverflow(node->arithMode())` is always true here (this is also asserted in DFGSpeculativeJIT.cpp).
We can remove this `|| !shouldCheckOverflow(node->arithMode()` part. By dropping this part, the code looks well aligned to ArithAdd's one.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:737
> +            setPrediction(speculationFromValue(m_currentNode->asJSValue()));

Nice.

[Saam Barati]

Comment on attachment 366907 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:768
>>                      break;
> 
> `shouldCheckOverflow(node->arithMode())` is always true here (this is also asserted in DFGSpeculativeJIT.cpp).
> We can remove this `|| !shouldCheckOverflow(node->arithMode()` part. By dropping this part, the code looks well aligned to ArithAdd's one.

This is really a different patch. Will fix in a follow up

[WebKit Commit Bot]

Comment on attachment 366907 [details]
patch

Clearing flags on attachment: 366907

Committed r244079: <https://trac.webkit.org/changeset/244079>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.