Bug 182214

Created attachment 332473 [details] Patch

Comment on attachment 332473 [details] Patch Attachment 332473 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6234382 New failing tests: stress/arith-negate-on-various-types.js.default stress/arith-negate-on-various-types.js.no-cjit-collect-continuously stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling-profiler stress/arith-negate-on-various-types.js.no-cjit-validate-phases stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o1 stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate stress/arith-negate-on-various-types.js.no-ftl stress/arith-negate-on-various-types.js.no-llint

Created attachment 332485 [details] Patch

Comment on attachment 332485 [details] Patch Attachment 332485 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6237172 New failing tests: fast/workers/worker-cloneport.html

Created attachment 332486 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Created attachment 333018 [details] Patch

Created attachment 333056 [details] Patch

Comment on attachment 333056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333056&action=review Some initial comments. I still need to review some parts in more detail. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:721 > + forNode(node).setType(m_graph, SpecBytecodeNumber | SpecCellOther); Why SpecCellOther and not BigInt directly? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-945 > - // We'd like to assert here that the arith profile for the result of negate never > - // sees a non-number, but we can't. It's true that negate never produces a non-number. > - // But sometimes we'll end up grabbing the wrong ArithProfile during OSR exit, and > - // profiling the wrong value, leading the ArithProfile to think it observed a non-number result. Why remove this comment? > Source/JavaScriptCore/jit/JITOperations.cpp:2686 > + JSValue result = JSValue(JSBigInt::unaryMinus(exec, asBigInt(primValue.asCell()))); why do you need the asCell here? We should juts add a asBigInt(JSValue) function if we don't already have one. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:408 > + if (UNLIKELY(throwScope.exception())) > + RETURN(JSValue()); I think you just want: EXCEPTION_CHECK() here > JSTests/stress/big-int-negate-jit.js:32 > +/* > +function negateBigInt(n) { > + return -n; > +} > +noInline(negateBigInt); > + > +for (let i = 0; i < 100000; i++) { > + assert(negateBigInt(100n), -100n); > + assert(negateBigInt(-0x1fffffffffffff01n), 0x1fffffffffffff01n); > +} > + > +if (numberOfDFGCompiles(negateBigInt) > 1) > + throw "Failed negateBigInt(). We should have compiled a single negate for the BigInt type."; > + > +function negateBigIntSpecializedToInt(n) { > + return -n; > +} > +noInline(negateBigIntSpecializedToInt); > + > +for (let i = 0; i < 100000; i++) { > + negateBigIntSpecializedToInt(100); > +} > + > +assert(negateBigIntSpecializedToInt(100n), -100n); > +*/ Why is this commented out?

Comment on attachment 333056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333056&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:721 >> + forNode(node).setType(m_graph, SpecBytecodeNumber | SpecCellOther); > > Why SpecCellOther and not BigInt directly? I see, we don't have SpecBigInt yet... We should add at some point, along with the necessary compiler changes.

Comment on attachment 333056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333056&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-945 >> - // profiling the wrong value, leading the ArithProfile to think it observed a non-number result. > > Why remove this comment? Ignore this. I see why you removed it.

Comment on attachment 333056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333056&action=review > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:201 > + SpeculatedType prediction = node->child1()->prediction(); > + if (prediction) { > + if (isInt32OrBooleanSpeculation(prediction) && node->canSpeculateInt32(m_pass)) > + changed |= mergePrediction(SpecInt32Only); > + else if (m_graph.unaryArithShouldSpeculateAnyInt(node, m_pass)) > + changed |= mergePrediction(SpecInt52Only); > + else if (isBytecodeNumberSpeculation(prediction)) > + changed |= mergePrediction(speculatedDoubleTypeForPrediction(node->child1()->prediction())); > + else { > + changed |= mergePrediction(SpecInt32Only); > + if (node->mayHaveNonNumberResult()) { > + // FIXME: We should add support to BigInt into speculation > + // https://bugs.webkit.org/show_bug.cgi?id=182470 > + changed |= mergePrediction(SpecCellOther); > + } > + if (node->mayHaveDoubleResult()) > + changed |= mergePrediction(SpecBytecodeDouble); > + } > + } > + break; This code is almost identical to the ArithNegate case below. Let's combine them in some way and perhaps just branch on node->op() where they differ.

Created attachment 335475 [details] Patch to test on EWS Testing in EWS. It includes code from https://bugs.webkit.org/show_bug.cgi?id=182470

Created attachment 337703 [details] Patch

Comment on attachment 337703 [details] Patch Attachment 337703 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7284294 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate

Comment on attachment 337703 [details] Patch Attachment 337703 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7284646 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

Created attachment 337718 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Created attachment 338072 [details] Patch

Comment on attachment 338072 [details] Patch Attachment 338072 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7338762 New failing tests: animations/needs-layout.html

Created attachment 338075 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

Ping Review

Ping Review

Created attachment 338579 [details] Patch Patch rebased.

Comment on attachment 338579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338579&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:740 > + forNode(node).setType(m_graph, SpecBytecodeNumber | SpecBigInt); Is this only used for BigInt? If not, why else would it be used? > Source/JavaScriptCore/runtime/JSBigInt.cpp:255 > + for (int i = 0; i < x->length(); i++) should be unsigned

Comment on attachment 338579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338579&action=review Thx for the review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:740 >> + forNode(node).setType(m_graph, SpecBytecodeNumber | SpecBigInt); > > Is this only used for BigInt? If not, why else would it be used? It is also used by Objects with valueOf or toString. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:255 >> + for (int i = 0; i < x->length(); i++) > > should be unsigned Ok

Created attachment 338956 [details] Patch updated Addressing Saam's comments.

Created attachment 339233 [details] Patch

Created attachment 339241 [details] Patch

Comment on attachment 339241 [details] Patch Attachment 339241 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7530964 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html

Created attachment 339254 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Ping?

Created attachment 339646 [details] Patch

Ping review

Created attachment 340157 [details] Patch

Created attachment 340263 [details] Patch

Comment on attachment 340263 [details] Patch Attachment 340263 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7669225 New failing tests: svg/custom/object-sizing-explicit-height.xhtml

Created attachment 340265 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 340263 [details] Patch Could you ensure performance regression is not introduced by running benchmarks?

(In reply to Yusuke Suzuki from comment #37) > Comment on attachment 340263 [details] > Patch > > Could you ensure performance regression is not introduced by running > benchmarks? Sure!

Created attachment 340270 [details] Benchmark results Here is the performance results. The patch is perf neutral in my machine.

Comment on attachment 340263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340263&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:766 > + forNode(node).setType(m_graph, SpecBytecodeNumber | SpecBigInt); Use setTypeForNode(node, SpecBytecodeNumber | SpecBigInt); > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:770 > case ArithNegate: { Should not have ArithNegate(Untyped) case. It is incorrect if the input is BigInt. It should be handled by ValueAdd. Let's add RELEASE_ASSERT_NOT_REACHED for "default" clause. > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:310 > - > + Remove this change > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:163 > + if (node->child1()->shouldSpeculateInt32OrBoolean() && node->canSpeculateInt32(FixupPass)) { > + node->setOp(ArithNegate); > + fixIntOrBooleanEdge(node->child1()); > + if (bytecodeCanTruncateInteger(node->arithNodeFlags())) > + node->setArithMode(Arith::Unchecked); > + else if (bytecodeCanIgnoreNegativeZero(node->arithNodeFlags())) > + node->setArithMode(Arith::CheckOverflow); > + else > + node->setArithMode(Arith::CheckOverflowAndNegativeZero); > + node->setResult(NodeResultInt32); > + node->clearFlags(NodeMustGenerate); > + break; > + } Nice. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:186 > + } And let's fix ArithNegate case too. ArithNegate can produce ArithNegate(Untyped). But it is wrong... ArithNegate(BigInt) would produce BigInt, but ArithNegate node type claims that it always produces numbers. So, we should remove ArithNegate(Untyped) case, and this should be handled ValueNegate(Untyped). This is the same strategy to ArithAdd and ValueAdd. We should fixup edges with some number filters in ArithNegate. If int fixup (Int32, Int52 etc.) is failed in ArithNegate, we should always call fixDoubleOrBooleanEdge(node->child1()) to emit edge filter. This is OK since bytecode parser emits ArithNegate only for operands which result is number. Original ArithNegate(Untyped) thing should be done by ValueNegate. > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:258 > else { We should have BigInt speculation type, edge filter, and speculation check here. Let's add FIXME and url. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4533 > + return; This "return" is unnecessary. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4536 > void SpeculativeJIT::compileArithNegate(Node* node) Why does ArithNegate have ArithNegate(Untyped)? Can we use ValueNegate for that case? Since ArithNegate's result type should be number, we cannot take BigInt for that. So, current ArithNegate(Untyped) is wrong with BigInt. ArithAdd does not have ArithAdd(Untyped, Untyped). Instead it uses ValueAdd(Untyped, Untyped) for that. Since ArithNegate is emitted in bytecode parser if the operand has NumberResult, and we only convert ValueAdd to ArithAdd with number related edge filtering, we can remove ArithNegate(Untyped) case if we carefully change fixup etc. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2720 > void compileArithNegate() Ditto. Remove ArithNegate(Untyped) case. And please ensure ArithAdd(Untyped) is not produced. > Source/JavaScriptCore/runtime/JSBigInt.cpp:253 > +JSBigInt* JSBigInt::copy(ExecState* state, JSBigInt* x) Take VM& instead of ExecState*. > Source/JavaScriptCore/runtime/JSBigInt.cpp:264 > +JSBigInt* JSBigInt::unaryMinus(ExecState* state, JSBigInt* x) Take VM& instead of ExecState*. > Source/JavaScriptCore/runtime/JSBigInt.h:81 > + enum ParseIntMode { DisallowEmptyString, AllowEmptyString }; > + enum ParseIntSign { Unsigned, Signed }; Use enum class.

Created attachment 340808 [details] Patch

Comment on attachment 340263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340263&action=review Thanks for the Review! >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:186 >> + } > > And let's fix ArithNegate case too. ArithNegate can produce ArithNegate(Untyped). But it is wrong... ArithNegate(BigInt) would produce BigInt, but ArithNegate node type claims that it always produces numbers. > So, we should remove ArithNegate(Untyped) case, and this should be handled ValueNegate(Untyped). This is the same strategy to ArithAdd and ValueAdd. > We should fixup edges with some number filters in ArithNegate. If int fixup (Int32, Int52 etc.) is failed in ArithNegate, we should always call fixDoubleOrBooleanEdge(node->child1()) to emit edge filter. > This is OK since bytecode parser emits ArithNegate only for operands which result is number. Original ArithNegate(Untyped) thing should be done by ValueNegate. Cool! You are right. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4536 >> void SpeculativeJIT::compileArithNegate(Node* node) > > Why does ArithNegate have ArithNegate(Untyped)? Can we use ValueNegate for that case? > Since ArithNegate's result type should be number, we cannot take BigInt for that. So, current ArithNegate(Untyped) is wrong with BigInt. > > ArithAdd does not have ArithAdd(Untyped, Untyped). Instead it uses ValueAdd(Untyped, Untyped) for that. Since ArithNegate is emitted in bytecode parser if the operand has NumberResult, and we only convert ValueAdd to ArithAdd with number related edge filtering, we can remove ArithNegate(Untyped) case if we carefully change fixup etc. Sure. Removing it.

Created attachment 340812 [details] Benchmark Report This is the new result of benchmarks. These changes are perf neutral.

Ping Review

Ping?

Comment on attachment 340808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340808&action=review r=me with comments. > Source/JavaScriptCore/jit/JITOperations.cpp:2720 > + return JSValue::encode(JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue)))); Do not need `JSValue()` wrap around JSBigInt result. `JSValue::encode(JSBigInt::xxx())` works. > Source/JavaScriptCore/jit/JITOperations.cpp:2740 > + JSValue result = JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue))); Do not need this `JSValue()` wrap. Just, JSBigInt* result = JSBigInt::xxx(); arithProfile.observeRusult(result); works. > Source/JavaScriptCore/jit/JITOperations.cpp:2785 > + JSValue result = JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue))); Ditto. > Source/JavaScriptCore/jit/JITOperations.cpp:2817 > + JSValue result = JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue))); Ditto. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:406 > + JSValue result = JSValue(JSBigInt::unaryMinus(exec->vm(), asBigInt(primValue))); Ditto.

Created attachment 341279 [details] Patch For landing Let's see EWS

Did you link the wrong bugzilla id in `// https://bugs.webkit.org/show_bug.cgi?id=18247`? It points to "On Mac: showModalDialog doesn't follow Apple's Human Interface Guidelines" right now.

(In reply to Dominik Inführ from comment #48) > Did you link the wrong bugzilla id in `// > https://bugs.webkit.org/show_bug.cgi?id=18247`? It points to "On Mac: > showModalDialog doesn't follow Apple's Human Interface Guidelines" right now. Nice Catch. I'm going to update it.

Created attachment 341440 [details] Patch for landing

Comment on attachment 341440 [details] Patch for landing Clearing flags on attachment: 341440 Committed r232232: <https://trac.webkit.org/changeset/232232>

All reviewed patches have been landed. Closing bug.

<rdar://problem/40590283>