[BigInt] Add ValueMod into DFG

[Caio Lima]

...

[Caio Lima]

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

[Caio Lima]

Created attachment 357312 [details]
WIP - Patch

[Caio Lima]

Created attachment 357558 [details]
WIP - Patch

[Caio Lima]

Created attachment 357559 [details]
Benchmarks

This patch seems perf neutral on x86_64

[Caio Lima]

Created attachment 357630 [details]
WIP - Patch

[Caio Lima]

Created attachment 357669 [details]
Patch

[Caio Lima]

Created attachment 357671 [details]
Patch

[Yusuke Suzuki]

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?

[Caio Lima]

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.

[Caio Lima]

Created attachment 358259 [details]
Patch

[Caio Lima]

Created attachment 358260 [details]
Benchmarks

Changed the Patch to have same Fixup rules for ValueDiv and ValueMod. This change seems perf neutral.

[Caio Lima]

Ping Review.

[Caio Lima]

Created attachment 359609 [details]
Patch

[Saam Barati]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 359875 [details]
Patch

[Caio Lima]

Created attachment 359877 [details]
Benchmarks

Patch seems perf neutral on x86_64.

[Caio Lima]

Created attachment 359973 [details]
Patch

[Caio Lima]

Created attachment 360005 [details]
Patch

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 360926 [details]
Patch

[Caio Lima]

Created attachment 362254 [details]
Patch

[Caio Lima]

Created attachment 364011 [details]
Patch

[Caio Lima]

Created attachment 367063 [details]
Patch

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 368939 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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

[Caio Lima]

(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.

[Saam Barati]

(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.

[Saam Barati]

Comment on attachment 368939 [details]
Patch

r=me

[Caio Lima]

Created attachment 369347 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 369381 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

(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.

[WebKit Commit Bot]

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

[Caio Lima]

Created attachment 369393 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 369393 [details]
Patch

Clearing flags on attachment: 369393

Committed r245063: <https://trac.webkit.org/changeset/245063>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50592127>