Bug 182470 - [ESNext][BigInt] Add support for BigInt in SpeculatedType
Summary: [ESNext][BigInt] Add support for BigInt in SpeculatedType
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:
Blocks: 179001 182214
  Show dependency treegraph
 
Reported: 2018-02-04 15:36 PST by Caio Lima
Modified: 2018-04-16 16:45 PDT (History)
11 users (show)

See Also:


Attachments
Patch (27.75 KB, patch)
2018-02-09 11:19 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (27.77 KB, patch)
2018-02-11 12:55 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (37.92 KB, patch)
2018-02-17 05:04 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (37.84 KB, patch)
2018-02-17 05:06 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (38.08 KB, patch)
2018-02-19 15:19 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (38.17 KB, patch)
2018-02-23 05:00 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (36.16 KB, patch)
2018-03-04 08:49 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (36.20 KB, patch)
2018-03-04 12:25 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (39.84 KB, patch)
2018-03-04 14:23 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (41.35 KB, patch)
2018-03-08 15:39 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (39.53 KB, patch)
2018-03-17 13:07 PDT, Caio Lima
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.01 MB, application/zip)
2018-03-17 15:35 PDT, Build Bot
no flags Details
Patch (39.62 KB, patch)
2018-03-21 15:54 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (41.64 KB, patch)
2018-03-25 13:09 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (41.91 KB, patch)
2018-03-29 04:15 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (41.93 KB, patch)
2018-04-10 21:23 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmark (89.55 KB, text/plain)
2018-04-10 22:19 PDT, Caio Lima
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2018-02-04 15:36:03 PST
...
Comment 1 Caio Lima 2018-02-09 11:19:06 PST
Created attachment 333502 [details]
Patch
Comment 2 Build Bot 2018-02-09 14:51:59 PST
Comment on attachment 333502 [details]
Patch

Attachment 333502 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6436599

New failing tests:
stress/arrowfunction-lexical-bind-superproperty.js.ftl-eager
Comment 3 Caio Lima 2018-02-11 12:55:26 PST
Created attachment 333571 [details]
Patch
Comment 4 Saam Barati 2018-02-13 15:27:30 PST
Comment on attachment 333571 [details]
Patch

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

r- just because of a few comments, but the patch LGTM overall. It'd be good to have some inferred type related tests.

> Source/JavaScriptCore/ChangeLog:3
> +        [ESNext][BigInt] We should add support to BigInt into speculation

"We should add support to BigInt into speculation" => "Add support for BigInt in SpeculatedType"

> Source/JavaScriptCore/ChangeLog:8
> +        This patch is introducing SpecBigInt type to DFG to enable BigInt

is introducing => introduces the

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10755
> +        case InferredType::BigInt:
> +            speculate(BadType, jsValueValue(value), edge.node(), isNotCell(value, provenType(edge)));
> +            speculate(BadType, jsValueValue(value), edge.node(), isNotBigInt(value, provenType(edge)));
> +            return;

Do you have a test for this?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14904
> +

You also need to add support in:
void speculate(Edge edge)

Should lead to compile time a crash, so you should add code that tests this.

> Source/JavaScriptCore/runtime/JSBigInt.h:40
> +    static const unsigned StructureFlags = Base::StructureFlags | OverridesToThis;

Why? You're not overriding it. Should you be?
Comment 5 Caio Lima 2018-02-14 02:43:12 PST
Comment on attachment 333571 [details]
Patch

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

Thank you for the review.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10755
>> +            return;
> 
> Do you have a test for this?

No. I don't think it can be tested into current implementation, since "checkInferredType" is only being called by MultiPutByOffset, and we don't emit this Node if one of the variants is not an Object. That is basically the case of all Cell primitives. And also I couldn't find any test covering other InferredType::(String|Symbol|Int...). I added this line to keep consistency, but maybe I'm missing something.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14904
>> +
> 
> You also need to add support in:
> void speculate(Edge edge)
> 
> Should lead to compile time a crash, so you should add code that tests this.

Oops. Thank you for that.

>> Source/JavaScriptCore/runtime/JSBigInt.h:40
>> +    static const unsigned StructureFlags = Base::StructureFlags | OverridesToThis;
> 
> Why? You're not overriding it. Should you be?

This flag is checked into ToThis when the code is in StrictMode to return the primitive value instead. If we don't do that, the test "JSTests/stress/big-int-strict-spec-to-this.js" will fail when DFG code is installed. This is also being set into Symbol without overriding the ToThis method. Is it not correct?
Comment 6 Caio Lima 2018-02-17 05:04:31 PST
Created attachment 334103 [details]
Patch
Comment 7 Caio Lima 2018-02-17 05:06:10 PST
Created attachment 334104 [details]
Patch
Comment 8 Build Bot 2018-02-17 06:21:05 PST
Comment on attachment 334104 [details]
Patch

Attachment 334104 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6551317

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
Comment 9 Caio Lima 2018-02-19 15:19:21 PST
Created attachment 334194 [details]
Patch
Comment 10 Caio Lima 2018-02-22 01:52:59 PST
Ping review
Comment 11 Caio Lima 2018-02-23 05:00:34 PST
Created attachment 334519 [details]
Patch
Comment 12 Caio Lima 2018-02-25 09:57:58 PST
Ping Review
Comment 13 Caio Lima 2018-03-04 08:49:38 PST
Created attachment 334981 [details]
Patch

Rebasing Patch
Comment 14 Caio Lima 2018-03-04 12:25:53 PST
Created attachment 334983 [details]
Patch

Fixing Bug found into compileBigIntEquality because it was missing speculation check if its operands are Cell.
Comment 15 Caio Lima 2018-03-04 14:23:47 PST
Created attachment 334984 [details]
Patch

Fixing build error into 32-bits.
Comment 16 Caio Lima 2018-03-06 15:17:03 PST
Ping Review?
Comment 17 Caio Lima 2018-03-08 15:39:39 PST
Created attachment 335357 [details]
Patch
Comment 18 Caio Lima 2018-03-17 13:07:58 PDT
Created attachment 336010 [details]
Patch

Rebasing Patch.
Comment 19 Build Bot 2018-03-17 15:35:01 PDT
Comment on attachment 336010 [details]
Patch

Attachment 336010 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7008446

New failing tests:
http/tests/preload/download_resources.html
Comment 20 Build Bot 2018-03-17 15:35:12 PDT
Created attachment 336015 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 21 Caio Lima 2018-03-21 15:54:54 PDT
Created attachment 336244 [details]
Patch

Rebasing Patch
Comment 22 Build Bot 2018-03-21 18:20:34 PDT
Comment on attachment 336244 [details]
Patch

Attachment 336244 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7057578

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
Comment 23 Caio Lima 2018-03-22 01:15:13 PDT
(In reply to Build Bot from comment #22)
> Comment on attachment 336244 [details]
> Patch
> 
> Attachment 336244 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/7057578
> 
> New failing tests:
> stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-
> cjit-validate

I tried reproduce this error locally, but it was not possible. I don't think it is related with my Patch.
Comment 24 Caio Lima 2018-03-23 01:00:09 PDT
Ping Review
Comment 25 Caio Lima 2018-03-25 13:09:26 PDT
Created attachment 336500 [details]
Patch
Comment 26 Saam Barati 2018-03-25 21:14:59 PDT
Comment on attachment 336500 [details]
Patch

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

r=me with comments/suggestions.

Also, there is a place inside DFGAbstractInterpreterInlines for CompareStrictEqual that you want to hook into. The line like:
 if (node->child1() == node->child2()) { ... }

Inside the ... you want to add a rule for BigInt speculations too.

> Source/JavaScriptCore/ChangeLog:15
> +        patches is to implement BigInt equallity check directly in

equallity => equality

> Source/JavaScriptCore/bytecode/SpeculatedType.h:71
> +static const SpeculatedType SpecCellOther          = 1ull << 26; // It's definitely a JSCell but not a subclass of JSObject and definitely not a JSString, BigInt or a Symbol.

JSString, BigInt or a Symbol => JSString, BigInt, or Symbol

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1471
> +        if (isBigIntSpeculation(abstractChild.m_type)) {
> +            setConstant(node, *m_graph.freeze(m_vm.smallStrings.bigintString()));
> +            break;
> +        }

Do you have a test for this?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2499
> +            if (node->child1()->shouldSpeculateBigInt()) {

Do you have a test for this?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5868
> +    callOperation(operationCompareStrictEqCell, resultGPR, leftGPR, rightGPR);

You're sure no exception will ever happen in this compare?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871
> +    m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR);

Why is this necessary?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6921
> +            LValue left = lowJSValue(m_node->child1(), ManualOperandSpeculation);
> +            LValue right = lowJSValue(m_node->child2(), ManualOperandSpeculation);

This is an anti pattern. You should add a lowBigInt function and use it instead of speculate() above. See lowSymbol, lowObject, etc.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6927
> +            ValueFromBlock fastResult = m_out.anchor(isEqualValue);

By making this the result, there is a chance B3/Air won't emit a fused compare and branch. It's worth ensuring it does. You can always make fastResult just be m_out.constInt32(1) if B3 doesn't emit a fused compare/branch. Actually, it's probably just making this one since we know the result if the compare is true.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6928
> +            m_out.branch(isEqualValue, rarely(continuation), usually(slowPath));

If this is actually rarely/usually, you may want to give these blocks different names.
Comment 27 Caio Lima 2018-03-29 03:28:17 PDT
Comment on attachment 336500 [details]
Patch

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

Thank you very much for the review and comments

>> Source/JavaScriptCore/ChangeLog:15
>> +        patches is to implement BigInt equallity check directly in
> 
> equallity => equality

Oops.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1471
>> +        }
> 
> Do you have a test for this?

Yes. It is the "JSTests/stress/big-int-strict-spec-to-this.js"

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2499
>> +            if (node->child1()->shouldSpeculateBigInt()) {
> 
> Do you have a test for this?

Yes. It is the "JSTests/stress/big-int-strict-spec-to-this.js"

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5868
>> +    callOperation(operationCompareStrictEqCell, resultGPR, leftGPR, rightGPR);
> 
> You're sure no exception will ever happen in this compare?

Yes. AFAIK, when we generate this code, both operands are BigInts and there is no exception on StrictEquals operation on BigInt.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871
>> +    m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR);
> 
> Why is this necessary?

I'm following the same approach of "SpeculativeJIT::nonSpeculativeNonPeepholeStrictEq" into "dfg/DFGSpeculativeJIT64.cpp". If I understand it correctly, it is sanitizing the result from "operationCompareStrictEqCell". Is it the real meaning of this operation? If not, I will remove in both places.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6921
>> +            LValue right = lowJSValue(m_node->child2(), ManualOperandSpeculation);
> 
> This is an anti pattern. You should add a lowBigInt function and use it instead of speculate() above. See lowSymbol, lowObject, etc.

Ok! I decided to do that because otherwise we won't have any other trace to test the case into "speculate(Edge edge)". But I agree that introducing lowBigInt makes more sense.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6927
>> +            ValueFromBlock fastResult = m_out.anchor(isEqualValue);
> 
> By making this the result, there is a chance B3/Air won't emit a fused compare and branch. It's worth ensuring it does. You can always make fastResult just be m_out.constInt32(1) if B3 doesn't emit a fused compare/branch. Actually, it's probably just making this one since we know the result if the compare is true.

You are right. The comparison isn't being fused here. Thanks for that.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6928
>> +            m_out.branch(isEqualValue, rarely(continuation), usually(slowPath));
> 
> If this is actually rarely/usually, you may want to give these blocks different names.

Done.
Comment 28 Caio Lima 2018-03-29 04:15:48 PDT
Created attachment 336764 [details]
Patch
Comment 29 Caio Lima 2018-04-08 05:01:06 PDT
Ping. I have a question before committing this Patch.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871
>> +    m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR);
> 
> Why is this necessary?

I'm following the same approach of "SpeculativeJIT::nonSpeculativeNonPeepholeStrictEq" into "dfg/DFGSpeculativeJIT64.cpp". If I understand it correctly, it is sanitizing the result from "operationCompareStrictEqCell". Is it the real meaning of this operation? If not, I will remove in both places.
Comment 30 Saam Barati 2018-04-10 18:31:55 PDT
(In reply to Caio Lima from comment #29)
> Ping. I have a question before committing this Patch.
> 
> >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871
> >> +    m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR);
> > 
> > Why is this necessary?
> 
> I'm following the same approach of
> "SpeculativeJIT::nonSpeculativeNonPeepholeStrictEq" into
> "dfg/DFGSpeculativeJIT64.cpp". If I understand it correctly, it is
> sanitizing the result from "operationCompareStrictEqCell". Is it the real
> meaning of this operation? If not, I will remove in both places.

It seems that is indeed what it's doing. It just seems peculiar that it does that given we assume static_cast<size_t>(true)==1 elsewhere in the code. You can probably just keep it as is.
Comment 31 Caio Lima 2018-04-10 21:23:07 PDT
Created attachment 337674 [details]
Patch
Comment 32 Caio Lima 2018-04-10 22:19:22 PDT
Created attachment 337677 [details]
Benchmark

It is perf neutral
Comment 33 WebKit Commit Bot 2018-04-10 22:49:36 PDT
Comment on attachment 337674 [details]
Patch

Clearing flags on attachment: 337674

Committed r230516: <https://trac.webkit.org/changeset/230516>
Comment 34 WebKit Commit Bot 2018-04-10 22:49:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2018-04-10 22:51:09 PDT
<rdar://problem/39338249>
Comment 36 Tomas Popela 2018-04-12 02:24:39 PDT
I skipped the JIT tests when it isn't enabled in https://trac.webkit.org/changeset/230564
Comment 37 Saam Barati 2018-04-12 08:26:04 PDT
(In reply to Tomas Popela from comment #36)
> I skipped the JIT tests when it isn't enabled in
> https://trac.webkit.org/changeset/230564

Why? Just to make tests run faster?
Comment 38 Tomas Popela 2018-04-12 21:21:15 PDT
(In reply to Saam Barati from comment #37)
> (In reply to Tomas Popela from comment #36)
> > I skipped the JIT tests when it isn't enabled in
> > https://trac.webkit.org/changeset/230564
> 
> Why? Just to make tests run faster?

No, look at bug 182730, the tests that are using numberOfDFGCompiles() will fail without JIT enabled.
Comment 39 Saam Barati 2018-04-16 16:45:51 PDT
(In reply to Tomas Popela from comment #38)
> (In reply to Saam Barati from comment #37)
> > (In reply to Tomas Popela from comment #36)
> > > I skipped the JIT tests when it isn't enabled in
> > > https://trac.webkit.org/changeset/230564
> > 
> > Why? Just to make tests run faster?
> 
> No, look at bug 182730, the tests that are using numberOfDFGCompiles() will
> fail without JIT enabled.

Ah. These type of tests are really something else...

They fail for so many weird reasons. This LGTM