Bug 186174

*** Bug 192662 has been marked as a duplicate of this bug. ***

Created attachment 357312 [details] WIP - Patch

Created attachment 357558 [details] WIP - Patch

Created attachment 357559 [details] Benchmarks This patch seems perf neutral on x86_64

Created attachment 357630 [details] WIP - Patch

Created attachment 357669 [details] Patch

Created attachment 357671 [details] Patch

Comment on attachment 357671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357671&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:529 > + Edge& leftChild = node->child1(); > + Edge& rightChild = node->child2(); > + > + if (Node::shouldSpeculateBigInt(leftChild.node(), rightChild.node())) { > + fixEdge<BigIntUse>(leftChild); > + fixEdge<BigIntUse>(rightChild); > + break; > + } > + > + if (m_graph.binaryArithShouldSpeculateInt32(node, FixupPass)) { > + node->setOp(ArithMod); > + node->setResult(NodeResultNumber); > + fixupArithDivInt32(node, leftChild, rightChild); > + break; > + } > + > + if (leftChild->shouldSpeculateNotCell() && rightChild->shouldSpeculateNotCell()) { > + fixDoubleOrBooleanEdge(leftChild); > + fixDoubleOrBooleanEdge(rightChild); > + node->setOp(ArithMod); > + node->setResult(NodeResultDouble); > + break; > + } > + > + fixEdge<UntypedUse>(leftChild); > + fixEdge<UntypedUse>(rightChild); > + break; This code is different from ValueDiv code. Could you explain the story behind this difference?

Comment on attachment 357671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357671&action=review >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:529 >> + break; > > This code is different from ValueDiv code. Could you explain the story behind this difference? Rules like ArithDiv will make ValueMod(Boolean, ...) fall back to ValueMod(Untyped, Untyped), causing perf regression into some microbenchmarks. Because of this, we introduced the rule of speculating Double or Boolean when ```shouldSpeculateNotCell``` is true in both operands.

Created attachment 358259 [details] Patch

Created attachment 358260 [details] Benchmarks Changed the Patch to have same Fixup rules for ValueDiv and ValueMod. This change seems perf neutral.

Ping Review.

Created attachment 359609 [details] Patch

Comment on attachment 359609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359609&action=review Mostly LGTM, but you have a bug in doesGC() > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:920 > + case ValueMod: Would be nice to do some constant folding. It makes sense to abstract away the constant folding done in ArithDiv. Just because we have UntypedUse does not mean we can't constant fold. > Source/JavaScriptCore/dfg/DFGDoesGC.cpp:108 > + case ValueMod: This should return true when BigIntUse. Also, same for all of the above operations since they can allocate a BigInt. > Source/JavaScriptCore/dfg/DFGOperations.cpp:376 > + if (WTF::holds_alternative<JSBigInt*>(leftNumeric) || WTF::holds_alternative<JSBigInt*>(rightNumeric)) { > + if (WTF::holds_alternative<JSBigInt*>(leftNumeric) && WTF::holds_alternative<JSBigInt*>(rightNumeric)) > + RELEASE_AND_RETURN(scope, JSValue::encode(JSBigInt::remainder(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric)))); > + > + return throwVMTypeError(exec, scope, "Invalid mix of BigInt and other type in remainder operation."); > + } Nit: There is a lot of code w.r.t BigInts that use this same branch pattern. It'd be nice to just generalize it in some function that accepts lambdas.

Comment on attachment 359609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359609&action=review Thank you very much for the review! >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:920 >> + case ValueMod: > > Would be nice to do some constant folding. It makes sense to abstract away the constant folding done in ArithDiv. Just because we have UntypedUse does not mean we can't constant fold. Ok. I'll work on that. >> Source/JavaScriptCore/dfg/DFGDoesGC.cpp:108 >> + case ValueMod: > > This should return true when BigIntUse. Also, same for all of the above operations since they can allocate a BigInt. Oops. Since we change the rule on Clobberize, I forgot to change this behavior as well. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:376 >> + } > > Nit: There is a lot of code w.r.t BigInts that use this same branch pattern. It'd be nice to just generalize it in some function that accepts lambdas. I agree. I want to try this in a separated patch, since last time I tried to implement with lambdas resulted in performance regressions.

Created attachment 359875 [details] Patch

Created attachment 359877 [details] Benchmarks Patch seems perf neutral on x86_64.

Created attachment 359973 [details] Patch

Created attachment 360005 [details] Patch

Ping Review

Created attachment 360926 [details] Patch

Created attachment 362254 [details] Patch

Created attachment 364011 [details] Patch

Created attachment 367063 [details] Patch

Ping Review

Created attachment 368939 [details] Patch

Comment on attachment 368939 [details] Patch Attachment 368939 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12089239 New failing tests: svg/repaint/remove-background-property-on-root.html

Created attachment 368952 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 368939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368939&action=review Mostly LGTM, but I think I found a constant folding bug. > Source/JavaScriptCore/ChangeLog:21 > + constant even if prediction propagation and fixup phases fail. nit: Those phase don't "fail". They predict types. I think you can just remove this sentence or rephrase to specify they picked less general types. > Source/JavaScriptCore/ChangeLog:38 > + ValueMod(BigIntUse) doesn't clobberize world because it only calls > + `operationModBigInt`. nice. > Source/JavaScriptCore/ChangeLog:45 > + ValueMod(BigIntUse) can trigger GC since it allocates intermediate > + JSBigInt to perform calculation. ValueMod(UntypedUse) can trigger GC > + because it can execute arbritary code from user. nice > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:237 > +bool AbstractInterpreter<AbstractStateType>::handleConstantDivOp(Node* node) I think you have a bug here where if we're doing BigInt Mod/Div, we need to ensure the constants are bigint. Like, it's wrong to constant fold this if lhs/rhs are int32, but we are speculating BigInt. Would be good to have a test for this. I believe you can create a test using Yusuke's fuzzer agent infrastructure. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5229 > + if (leftChild.useKind() == BigIntUse && rightChild.useKind() == BigIntUse) { nit: I think you can use node->binaryUseKind() == BigIntUse

(In reply to Saam Barati from comment #29) > Comment on attachment 368939 [details] > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:237 > > +bool AbstractInterpreter<AbstractStateType>::handleConstantDivOp(Node* node) > > I think you have a bug here where if we're doing BigInt Mod/Div, we need to > ensure the constants are bigint. Like, it's wrong to constant fold this if > lhs/rhs are int32, but we are speculating BigInt. > > Would be good to have a test for this. I believe you can create a test using > Yusuke's fuzzer agent infrastructure. Oh right. With this I can force a scenario to have the following code ``` @1 - JSConstant(Int32: 10) @2 - JSConstant(Int32: 2) @3 - ValueMod(BigInt, @1, @2) ``` However I don't see how it is possible to have a case described above, since `executeEdges(node)` will make impossible to prove that @1 and @2 are int32 (or Number) while we are speculating BigInt. I agree that `AbstractInterpreter::executeEffects` shouldn't rely on `executeEdges` and this check is necessary, but I don't know how to add a test where AI proves children as Int32 having `node->binaryUseKind() == BigInt`. IIUC, it is not possible to have this case during constant folding, but we can have this issue on other analysis that rely on AI.

(In reply to Caio Lima from comment #30) > (In reply to Saam Barati from comment #29) > > Comment on attachment 368939 [details] > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:237 > > > +bool AbstractInterpreter<AbstractStateType>::handleConstantDivOp(Node* node) > > > > I think you have a bug here where if we're doing BigInt Mod/Div, we need to > > ensure the constants are bigint. Like, it's wrong to constant fold this if > > lhs/rhs are int32, but we are speculating BigInt. > > > > Would be good to have a test for this. I believe you can create a test using > > Yusuke's fuzzer agent infrastructure. > > Oh right. With this I can force a scenario to have the following code > ``` > @1 - JSConstant(Int32: 10) > @2 - JSConstant(Int32: 2) > @3 - ValueMod(BigInt, @1, @2) > ``` > > However I don't see how it is possible to have a case described above, since > `executeEdges(node)` will make impossible to prove that @1 and @2 are int32 > (or Number) while we are speculating BigInt. I agree that > `AbstractInterpreter::executeEffects` shouldn't rely on `executeEdges` and > this check is necessary, but I don't know how to add a test where AI proves > children as Int32 having `node->binaryUseKind() == BigInt`. IIUC, it is not > possible to have this case during constant folding, but we can have this > issue on other analysis that rely on AI. Yeah your correct. We'll end up calling AbstractValue::clear() in such a scenario.

Comment on attachment 368939 [details] Patch r=me

Created attachment 369347 [details] Patch

Comment on attachment 369347 [details] Patch Attachment 369347 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12130680 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html

Created attachment 369361 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 369381 [details] Patch

Comment on attachment 369381 [details] Patch Attachment 369381 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12133528 New failing tests: svg/repaint/remove-border-property-on-root.html http/tests/css/filters-on-iframes.html

Created attachment 369390 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

(In reply to Build Bot from comment #37) > Comment on attachment 369381 [details] > Patch > > Attachment 369381 [details] did not pass win-ews (win): > Output: https://webkit-queues.webkit.org/results/12133528 > > New failing tests: > svg/repaint/remove-border-property-on-root.html > http/tests/css/filters-on-iframes.html I think these failures are unrelated with the patch.

Comment on attachment 369381 [details] Patch Rejecting attachment 369381 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 369381, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in PerformanceTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/12134158

Created attachment 369393 [details] Patch

Comment on attachment 369393 [details] Patch Clearing flags on attachment: 369393 Committed r245063: <https://trac.webkit.org/changeset/245063>

All reviewed patches have been landed. Closing bug.

<rdar://problem/50592127>