Bug 196639

Summary: Clean up Int52 code and some bugs in it
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 196668    
Attachments:
Description Flags
WIP
none
WIP
none
WIP
none
patch none

Description Saam Barati 2019-04-04 19:37:05 PDT
...
Comment 1 Saam Barati 2019-04-04 20:11:55 PDT
Created attachment 366792 [details]
WIP

It begins.
Comment 2 Saam Barati 2019-04-05 12:37:22 PDT
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).
Comment 3 Saam Barati 2019-04-05 12:57:04 PDT
Most places that use (2) are doing the right thing. But some are probably being too conservative.
Comment 4 Saam Barati 2019-04-05 17:53:53 PDT
Created attachment 366863 [details]
WIP

I'm just moving things to a segregated Int52 lattice
Comment 5 Saam Barati 2019-04-05 20:08:12 PDT
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.
Comment 6 Saam Barati 2019-04-05 20:08:40 PDT
(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.
Comment 7 EWS Watchlist 2019-04-05 20:57:15 PDT
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.
Comment 8 Saam Barati 2019-04-07 12:16:29 PDT
<rdar://problem/49515757>
Comment 9 Saam Barati 2019-04-07 12:19:09 PDT
Created attachment 366907 [details]
patch
Comment 10 EWS Watchlist 2019-04-07 12:21:20 PDT
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.
Comment 11 Yusuke Suzuki 2019-04-09 00:24:57 PDT
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.
Comment 12 Saam Barati 2019-04-09 08:50:34 PDT
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
Comment 13 WebKit Commit Bot 2019-04-09 09:06:54 PDT
Comment on attachment 366907 [details]
patch

Clearing flags on attachment: 366907

Committed r244079: <https://trac.webkit.org/changeset/244079>
Comment 14 WebKit Commit Bot 2019-04-09 09:06:55 PDT
All reviewed patches have been landed.  Closing bug.