...
Created attachment 366792 [details] WIP It begins.
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).
Most places that use (2) are doing the right thing. But some are probably being too conservative.
Created attachment 366863 [details] WIP I'm just moving things to a segregated Int52 lattice
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.
(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.
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.
<rdar://problem/49515757>
Created attachment 366907 [details] patch
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 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 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 on attachment 366907 [details] patch Clearing flags on attachment: 366907 Committed r244079: <https://trac.webkit.org/changeset/244079>
All reviewed patches have been landed. Closing bug.