With the introduction of BigInt, emitting BitNotNode as x ^ -1 is not safe anymore. This way, we need to re-introduce op_bitnot.
Created attachment 354831 [details] WIP - Patch
Comment on attachment 354831 [details] WIP - Patch Attachment 354831 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9992336 New failing tests: js/kde/md5-2.html
Created attachment 354843 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 354881 [details] WIP - Patch
Comment on attachment 354881 [details] WIP - Patch Attachment 354881 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9998041 New failing tests: js/kde/md5-2.html
Created attachment 354887 [details] Archive of layout-test-results from ews203 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews203 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 354881 [details] WIP - Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354881&action=review I think this direction is good. Some nits before the review. > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:1053 > + void not32(RegisterID srcDest, RegisterID) The second argument is not necessary. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3539 > + m_jit.not32(resultGPR, resultGPR); Use `not32(resultGPR);`.
Comment on attachment 354881 [details] WIP - Patch Attachment 354881 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9998659 New failing tests: js/kde/md5-2.html
Created attachment 354891 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 354933 [details] EWS win test
Comment on attachment 354933 [details] EWS win test Attachment 354933 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10004727 New failing tests: js/kde/md5-2.html
Created attachment 354952 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 354933 [details] EWS win test Attachment 354933 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10005721 New failing tests: js/kde/md5-2.html
Created attachment 354964 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 354989 [details] Patch
Created attachment 354991 [details] Patch to test ews
Created attachment 355043 [details] Patch
Created attachment 355141 [details] Patch
Comment on attachment 355141 [details] Patch Attachment 355141 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10027970 New failing tests: sputnik/Conformance/11_Expressions/11.4_Unary_Operators/11.4.8_Bitwise_NOT_Operator/S11.4.8_A2.1_T1.html js/kde/md5-2.html
Created attachment 355171 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 355175 [details] Patch
Comment on attachment 355175 [details] Patch Attachment 355175 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10029681 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
Created attachment 355184 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 355198 [details] Patch
Created attachment 355203 [details] Patch for news
Comment on attachment 355203 [details] Patch for news Attachment 355203 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10047326 New failing tests: sputnik/Conformance/11_Expressions/11.4_Unary_Operators/11.4.8_Bitwise_NOT_Operator/S11.4.8_A2.1_T1.html js/kde/md5-2.html
Created attachment 355212 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 355216 [details] Patch to test ews
Created attachment 355241 [details] WIP - Patch
Comment on attachment 355241 [details] WIP - Patch Rebasing with master
Created attachment 355277 [details] WIP - Patch
Created attachment 355280 [details] WIP - Patch
Created attachment 355430 [details] Patch
Created attachment 355636 [details] Patch
Comment on attachment 355636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355636&action=review r=me with nits. Please ensure it does not cause any performance regression before landing. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:388 > + setConstant(node, JSValue(~a)); Insert `break;` here. > Source/JavaScriptCore/dfg/DFGNodeType.h:114 > + macro(ArithBitNot, NodeResultInt32) \ Add `NodeMustGenerate` since the Untyped case can cause anything. And clear `NodeMustGenerate` when the edge is fixed by `Int32` in the fixup phase. Later, if you add ValueBitNot, then you can make this non-MustGenerate, and handle Untyped cases in ValueBitNot.
Created attachment 355689 [details] Benchmarks The Patch is perf neutral on x86_64 macOS.
Created attachment 355698 [details] Patch
Thank you very much for the review! (In reply to Yusuke Suzuki from comment #35) > Comment on attachment 355636 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=355636&action=review > > r=me with nits. Please ensure it does not cause any performance regression > before landing. > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:388 > > + setConstant(node, JSValue(~a)); > > Insert `break;` here. Done! > > Source/JavaScriptCore/dfg/DFGNodeType.h:114 > > + macro(ArithBitNot, NodeResultInt32) \ > > Add `NodeMustGenerate` since the Untyped case can cause anything. > And clear `NodeMustGenerate` when the edge is fixed by `Int32` in the fixup > phase. > Later, if you add ValueBitNot, then you can make this non-MustGenerate, and > handle Untyped cases in ValueBitNot. Done. I also added a test case. thx for catching this.
The commit-queue encountered the following flaky tests while processing attachment 355698 [details]: webgl/2.0.0/conformance2/rendering/blitframebuffer-filter-outofbounds.html bug 191991 (author: justin_fan@apple.com) webgl/2.0.0/conformance/more/functions/readPixels.html bug 191992 (author: justin_fan@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 355698 [details] Patch Clearing flags on attachment: 355698 Committed r238543: <https://trac.webkit.org/changeset/238543>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46264064>
r238543 introduced a regression on Armv7. See Bug 192152.
Comment on attachment 355698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355698&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:390 > + JSValue operand = forNode(node->child1()).value(); > + if (operand && operand.isInt32()) { > + int32_t a = operand.asInt32(); > + setConstant(node, JSValue(~a)); > + break; > + } This constant folding can happen irrespective of being UntypedUse or Int32Use
Comment on attachment 355698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355698&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855 > + LValue result = vmCall(pointerType(), m_out.operation(operationValueBitNot), m_callFrame, operand); nit: Should be Int64 not pointerType()
(In reply to Saam Barati from comment #45) > Comment on attachment 355698 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=355698&action=review > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855 > > + LValue result = vmCall(pointerType(), m_out.operation(operationValueBitNot), m_callFrame, operand); > > nit: Should be Int64 not pointerType() I will handle both comments into https://bugs.webkit.org/show_bug.cgi?id=182216
(In reply to Guillaume Emont from comment #43) > r238543 introduced a regression on Armv7. See Bug 192152. This probably will be fixed by https://bugs.webkit.org/show_bug.cgi?id=192203