Bug 196639

Summary: Clean up Int52 code and some bugs in it
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
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

Saam Barati
Reported 2019-04-04 19:37:05 PDT
...
Attachments
WIP (15.27 KB, patch)
2019-04-04 20:11 PDT, Saam Barati
no flags
WIP (51.68 KB, patch)
2019-04-05 17:53 PDT, Saam Barati
no flags
WIP (53.33 KB, patch)
2019-04-05 20:08 PDT, Saam Barati
no flags
patch (56.86 KB, patch)
2019-04-07 12:19 PDT, Saam Barati
no flags
Saam Barati
Comment 1 2019-04-04 20:11:55 PDT
Created attachment 366792 [details] WIP It begins.
Saam Barati
Comment 2 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).
Saam Barati
Comment 3 2019-04-05 12:57:04 PDT
Most places that use (2) are doing the right thing. But some are probably being too conservative.
Saam Barati
Comment 4 2019-04-05 17:53:53 PDT
Created attachment 366863 [details] WIP I'm just moving things to a segregated Int52 lattice
Saam Barati
Comment 5 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.
Saam Barati
Comment 6 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.
EWS Watchlist
Comment 7 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.
Saam Barati
Comment 8 2019-04-07 12:16:29 PDT
Saam Barati
Comment 9 2019-04-07 12:19:09 PDT
EWS Watchlist
Comment 10 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.
Yusuke Suzuki
Comment 11 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.
Saam Barati
Comment 12 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
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2019-04-09 09:06:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.