Bug 186176 - [BigInt] Add ValueSub into DFG
Summary: [BigInt] Add ValueSub into DFG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords: InRadar
Depends on: 186177 190701
Blocks: 186173
  Show dependency treegraph
 
Reported: 2018-05-31 19:10 PDT by Caio Lima
Modified: 2019-01-21 04:49 PST (History)
8 users (show)

See Also:


Attachments
WIP -It starts (20.07 KB, patch)
2018-07-01 23:07 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch (20.03 KB, patch)
2018-10-09 06:37 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-sierra (3.07 MB, application/zip)
2018-10-09 08:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.04 MB, application/zip)
2018-10-09 10:40 PDT, Build Bot
no flags Details
WIP - Patch (24.98 KB, patch)
2018-10-09 14:14 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (25.39 KB, patch)
2018-10-15 15:55 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (25.37 KB, patch)
2018-10-17 17:33 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (26.01 KB, patch)
2018-10-18 22:06 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (26.04 KB, patch)
2018-10-19 05:32 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2018-05-31 19:10:31 PDT
...
Comment 1 Caio Lima 2018-07-01 23:07:23 PDT
Created attachment 344077 [details]
WIP -It starts

This Patch is not complete  yet.
Comment 2 Caio Lima 2018-10-09 06:37:52 PDT
Created attachment 351876 [details]
WIP - Patch
Comment 3 Build Bot 2018-10-09 08:42:39 PDT
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
Comment 4 Build Bot 2018-10-09 08:42:40 PDT
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
Comment 5 Build Bot 2018-10-09 10:40:22 PDT
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
Comment 6 Build Bot 2018-10-09 10:40:23 PDT
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
Comment 7 Caio Lima 2018-10-09 14:14:24 PDT
Created attachment 351914 [details]
WIP - Patch
Comment 8 Yusuke Suzuki 2018-10-10 04:22:38 PDT
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.
Comment 9 Caio Lima 2018-10-10 06:56:36 PDT
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.
Comment 10 Yusuke Suzuki 2018-10-10 07:05:01 PDT
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.
Comment 11 Yusuke Suzuki 2018-10-10 07:10:18 PDT
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.
Comment 12 Caio Lima 2018-10-15 15:55:46 PDT
Created attachment 352395 [details]
Patch
Comment 13 Yusuke Suzuki 2018-10-17 06:47:48 PDT
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.
Comment 14 Caio Lima 2018-10-17 17:33:13 PDT
Created attachment 352666 [details]
Patch
Comment 15 Caio Lima 2018-10-17 17:37:00 PDT
Thx for the review!
Comment 16 WebKit Commit Bot 2018-10-17 18:14:08 PDT
Comment on attachment 352666 [details]
Patch

Clearing flags on attachment: 352666

Committed r237242: <https://trac.webkit.org/changeset/237242>
Comment 17 WebKit Commit Bot 2018-10-17 18:14:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2018-10-17 18:15:56 PDT
<rdar://problem/45358866>
Comment 19 WebKit Commit Bot 2018-10-18 04:19:45 PDT
Re-opened since this is blocked by bug 190701
Comment 20 Caio Lima 2018-10-18 22:06:42 PDT
Created attachment 352768 [details]
Patch
Comment 21 Caio Lima 2018-10-18 22:12:51 PDT
@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.
Comment 22 Yusuke Suzuki 2018-10-19 00:30:35 PDT
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.
Comment 23 Caio Lima 2018-10-19 05:32:38 PDT
Created attachment 352782 [details]
Patch
Comment 24 Caio Lima 2018-10-19 06:49:13 PDT
Thx agin for the review.
Comment 25 WebKit Commit Bot 2018-10-19 06:55:42 PDT
Comment on attachment 352782 [details]
Patch

Clearing flags on attachment: 352782

Committed r237285: <https://trac.webkit.org/changeset/237285>
Comment 26 WebKit Commit Bot 2018-10-19 06:55:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Saam Barati 2019-01-20 13:23:37 PST
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?
Comment 28 Caio Lima 2019-01-21 04:49:43 PST
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?