Bug 186176 – [BigInt] Add ValueSub into DFG

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

...

[Caio Lima]

Created attachment 344077 [details]
WIP -It starts

This Patch is not complete  yet.

[Caio Lima]

Created attachment 351876 [details]
WIP - Patch

[EWS Watchlist]

Comment on attachment 351876 [details]
WIP - Patch

Attachment 351876 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9503455

New failing tests:
webgl/2.0.0/conformance/uniforms/out-of-bounds-uniform-array-access.html
webgl/1.0.2/conformance/uniforms/out-of-bounds-uniform-array-access.html

[EWS Watchlist]

Created attachment 351880 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351876 [details]
WIP - Patch

Attachment 351876 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9504348

New failing tests:
webgl/2.0.0/conformance/uniforms/out-of-bounds-uniform-array-access.html
webgl/1.0.2/conformance/uniforms/out-of-bounds-uniform-array-access.html

[EWS Watchlist]

Created attachment 351890 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Caio Lima]

Created attachment 351914 [details]
WIP - Patch

[Yusuke Suzuki]

Comment on attachment 351914 [details]
WIP - Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119
> +                node->setOp(ArithSub);

You shoud use setOpAndDefaultFlags since it does not clear MustGenerate.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:126
> +            node->setOp(ArithSub);

Ditto.

[Caio Lima]

Comment on attachment 351914 [details]
WIP - Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review

Thx @Yusuke for the informal review. I left some questions about your review.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119
>> +                node->setOp(ArithSub);
> 
> You shoud use setOpAndDefaultFlags since it does not clear MustGenerate.

IIRC, I think using this will make this code wrong, because setOpAndDefaultFlags will reset this node result and ```attemptToMakeIntegerAdd``` sets properly expected result in some cases.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:126
>> +            node->setOp(ArithSub);
> 
> Ditto.

I'm not sure if you mean to remove NodeResultDouble and use ```setOpAndDefaultFlags```, but ArithSub default flag is ```NodeResultNumber``` instead of ```NodeResultDouble```. Could you confirm that? Using ```setOpAndDefaultFlags``` instead of ```setOp + setResult``` makes a bunch of test crash.

[Yusuke Suzuki]

Comment on attachment 351914 [details]
WIP - Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review

>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119
>>> +                node->setOp(ArithSub);
>> 
>> You shoud use setOpAndDefaultFlags since it does not clear MustGenerate.
> 
> IIRC, I think using this will make this code wrong, because setOpAndDefaultFlags will reset this node result and ```attemptToMakeIntegerAdd``` sets properly expected result in some cases.

Make sense. Then, you need to clear MustGenerate by using clearFlags(NodeMustGenerate).

>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:126
>>> +            node->setOp(ArithSub);
>> 
>> Ditto.
> 
> I'm not sure if you mean to remove NodeResultDouble and use ```setOpAndDefaultFlags```, but ArithSub default flag is ```NodeResultNumber``` instead of ```NodeResultDouble```. Could you confirm that? Using ```setOpAndDefaultFlags``` instead of ```setOp + setResult``` makes a bunch of test crash.

Ditto. You need to clear NodeMustGenerate flag.

[Yusuke Suzuki]

Comment on attachment 351914 [details]
WIP - Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351914&action=review

>>>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:119
>>>> +                node->setOp(ArithSub);
>>> 
>>> You shoud use setOpAndDefaultFlags since it does not clear MustGenerate.
>> 
>> IIRC, I think using this will make this code wrong, because setOpAndDefaultFlags will reset this node result and ```attemptToMakeIntegerAdd``` sets properly expected result in some cases.
> 
> Make sense. Then, you need to clear MustGenerate by using clearFlags(NodeMustGenerate).

After looking into the code of attemptToMakeIntegerAdd, I've just understood the problem. ArithSub can have checks (for overflow), so, MustGenerate cannot be cleared easily. (Only Unchecked situation can drop MustGenerate).
Because the original ArithSub node has MustGenerate flag anyway, clearing it would be done in a separate patch.

[Caio Lima]

Created attachment 352395 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 352395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352395&action=review

r=me with nit.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:595
> +        ASSERT(node->binaryUseKind() == UntypedUse || node->binaryUseKind() == BigIntUse);

Let's use DFG_ASSERT.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:125
> +            ASSERT(Node::shouldSpeculateNumberOrBooleanExpectingDefined(child1.node(), child2.node()));

Let's use DFG_ASSERT.

[Caio Lima]

Created attachment 352666 [details]
Patch

[Caio Lima]

Thx for the review!

[WebKit Commit Bot]

Comment on attachment 352666 [details]
Patch

Clearing flags on attachment: 352666

Committed r237242: <https://trac.webkit.org/changeset/237242>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45358866>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 190701

[Caio Lima]

Created attachment 352768 [details]
Patch

[Caio Lima]

@Yusuke.

I updated this patch comparing to the previous one removing the assert "Node::shouldSpeculateNumberOrBooleanExpectingDefined(child1.node(), child2.node())" from DFGFixupPhase.cpp. The point is that the following assert is not true. I created the test ```JSTests/stress/value-sub-spec-none-case.js``` to show an edge case that will cause the assertion to fail. In this test, ValueSub will have one operand as SpecNone and other as Double. Even though we are fixing edge as double or boolean, this  fallback is still safe, because such cases will generate ForceOSRExit before the execution of ValueSub/ArithSub.

[Yusuke Suzuki]

Comment on attachment 352768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352768&action=review

r=me

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:120
> +                // FIXME: [DFG] Clear ArithSub when ArithMode is Unchecked
> +                // https://bugs.webkit.org/show_bug.cgi?id=190607

Ditto.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:327
> +            // FIXME: [DFG] Clear ArithSub when ArithMode is Unchecked

Fix the description to "Clear ArithSub's NodeMustGenerate when ArithMode is unchecked". And `[DFG]` is not necessary.

[Caio Lima]

Created attachment 352782 [details]
Patch

[Caio Lima]

Thx agin for the review.

[WebKit Commit Bot]

Comment on attachment 352782 [details]
Patch

Clearing flags on attachment: 352782

Committed r237285: <https://trac.webkit.org/changeset/237285>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 352782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352782&action=review

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:596
>          clobberWorld();

this isn't true when we're BigIntUse

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:600
> +        if (node->binaryUseKind() == BigIntUse)
> +            setTypeForNode(node, SpecBigInt);
> +        else
> +            setTypeForNode(node, SpecString | SpecBytecodeNumber | SpecBigInt);

We should do some constant folding here. Profiling information may not be perfect, and AI could prove these values as constants.

> Source/JavaScriptCore/dfg/DFGClobberize.h:642
> +    case ValueSub:

This should be PureValue when BigIntUse.

> Source/JavaScriptCore/dfg/DFGDoesGC.cpp:102
> +    case ValueSub:

With the other changes I propose, this should return `true` for BigIntUse

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:108
> +                fixEdge<BigIntUse>(child2);

We should clear MustGenerate here I believe.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:127
> +            fixDoubleOrBooleanEdge(node->child1());
> +            fixDoubleOrBooleanEdge(node->child2());
> +            node->setOp(ArithSub);

Should we clear MustGenerate here?

[Caio Lima]

Comment on attachment 352782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352782&action=review

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:596
>>          clobberWorld();
> 
> this isn't true when we're BigIntUse

We changed this on ToT already.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:600
>> +            setTypeForNode(node, SpecString | SpecBytecodeNumber | SpecBigInt);
> 
> We should do some constant folding here. Profiling information may not be perfect, and AI could prove these values as constants.

This is good to know. I'll do that in coming patches.

>> Source/JavaScriptCore/dfg/DFGClobberize.h:642
>> +    case ValueSub:
> 
> This should be PureValue when BigIntUse.

It is already done on ToT.

>> Source/JavaScriptCore/dfg/DFGDoesGC.cpp:102
>> +    case ValueSub:
> 
> With the other changes I propose, this should return `true` for BigIntUse

I'll change that. Right now we actually have wrong rules for every Op.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:108
>> +                fixEdge<BigIntUse>(child2);
> 
> We should clear MustGenerate here I believe.

I think it is ok.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:127
>> +            node->setOp(ArithSub);
> 
> Should we clear MustGenerate here?

I'm not sure. Can we have issues with neg zero here?