[webkit-reviews] review denied: [Bug 186174] [BigInt] Add ValueMod into DFG : [Attachment 368939] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 186174: [BigInt] Add ValueMod into DFG
https://bugs.webkit.org/show_bug.cgi?id=186174

Attachment 368939: Patch

https://bugs.webkit.org/attachment.cgi?id=368939&action=review




--- Comment #29 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 368939
  --> https://bugs.webkit.org/attachment.cgi?id=368939
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