Bug 182470 – [ESNext][BigInt] Add support for BigInt in SpeculatedType

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

[Caio Lima]

...

[Caio Lima]

Created attachment 333502 [details]
Patch

[EWS Watchlist]

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

[Caio Lima]

Created attachment 333571 [details]
Patch

[Saam Barati]

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?

[Caio Lima]

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?

[Caio Lima]

Created attachment 334103 [details]
Patch

[Caio Lima]

Created attachment 334104 [details]
Patch

[EWS Watchlist]

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

[Caio Lima]

Created attachment 334194 [details]
Patch

[Caio Lima]

Ping review

[Caio Lima]

Created attachment 334519 [details]
Patch

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 334981 [details]
Patch

Rebasing Patch

[Caio Lima]

Created attachment 334983 [details]
Patch

Fixing Bug found into compileBigIntEquality because it was missing speculation check if its operands are Cell.

[Caio Lima]

Created attachment 334984 [details]
Patch

Fixing build error into 32-bits.

[Caio Lima]

Ping Review?

[Caio Lima]

Created attachment 335357 [details]
Patch

[Caio Lima]

Created attachment 336010 [details]
Patch

Rebasing Patch.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 336244 [details]
Patch

Rebasing Patch

[EWS Watchlist]

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

[Caio Lima]

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

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 336500 [details]
Patch

[Saam Barati]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 336764 [details]
Patch

[Caio Lima]

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.

[Saam Barati]

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

[Caio Lima]

Created attachment 337674 [details]
Patch

[Caio Lima]

Created attachment 337677 [details]
Benchmark

It is perf neutral

[WebKit Commit Bot]

Comment on attachment 337674 [details]
Patch

Clearing flags on attachment: 337674

Committed r230516: <https://trac.webkit.org/changeset/230516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39338249>

[Tomas Popela]

I skipped the JIT tests when it isn't enabled in https://trac.webkit.org/changeset/230564

[Saam Barati]

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

[Tomas Popela]

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

[Saam Barati]

(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