Bug 182214 – [ESNext][BigInt] Implement "+" and "-" unary operation

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

[Caio Lima]

...

[Caio Lima]

Created attachment 332473 [details]
Patch

[EWS Watchlist]

Comment on attachment 332473 [details]
Patch

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

New failing tests:
stress/arith-negate-on-various-types.js.default
stress/arith-negate-on-various-types.js.no-cjit-collect-continuously
stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling-profiler
stress/arith-negate-on-various-types.js.no-cjit-validate-phases
stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o1
stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate
stress/arith-negate-on-various-types.js.no-ftl
stress/arith-negate-on-various-types.js.no-llint

[Caio Lima]

Created attachment 332485 [details]
Patch

[EWS Watchlist]

Comment on attachment 332485 [details]
Patch

Attachment 332485 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6237172

New failing tests:
fast/workers/worker-cloneport.html

[EWS Watchlist]

Created attachment 332486 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

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

[Caio Lima]

Created attachment 333018 [details]
Patch

[Caio Lima]

Created attachment 333056 [details]
Patch

[Saam Barati]

Comment on attachment 333056 [details]
Patch

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

Some initial comments. I still need to review some parts in more detail.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:721
> +        forNode(node).setType(m_graph, SpecBytecodeNumber | SpecCellOther);

Why SpecCellOther and not BigInt directly?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-945
> -                    // We'd like to assert here that the arith profile for the result of negate never
> -                    // sees a non-number, but we can't. It's true that negate never produces a non-number.
> -                    // But sometimes we'll end up grabbing the wrong ArithProfile during OSR exit, and
> -                    // profiling the wrong value, leading the ArithProfile to think it observed a non-number result.

Why remove this comment?

> Source/JavaScriptCore/jit/JITOperations.cpp:2686
> +        JSValue result = JSValue(JSBigInt::unaryMinus(exec, asBigInt(primValue.asCell())));

why do you need the asCell here? We should juts add a 
asBigInt(JSValue)
function if we don't already have one.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:408
> +    if (UNLIKELY(throwScope.exception()))
> +        RETURN(JSValue());

I think you just want:
EXCEPTION_CHECK()
here

> JSTests/stress/big-int-negate-jit.js:32
> +/*
> +function negateBigInt(n) {
> +    return -n;
> +}
> +noInline(negateBigInt);
> +
> +for (let i = 0; i < 100000; i++) {
> +    assert(negateBigInt(100n), -100n);
> +    assert(negateBigInt(-0x1fffffffffffff01n), 0x1fffffffffffff01n);
> +}
> +
> +if (numberOfDFGCompiles(negateBigInt) > 1)
> +    throw "Failed negateBigInt(). We should have compiled a single negate for the BigInt type.";
> +
> +function negateBigIntSpecializedToInt(n) {
> +    return -n;
> +}
> +noInline(negateBigIntSpecializedToInt);
> +
> +for (let i = 0; i < 100000; i++) {
> +    negateBigIntSpecializedToInt(100);
> +}
> +
> +assert(negateBigIntSpecializedToInt(100n), -100n);
> +*/

Why is this commented out?

[Saam Barati]

Comment on attachment 333056 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:721
>> +        forNode(node).setType(m_graph, SpecBytecodeNumber | SpecCellOther);
> 
> Why SpecCellOther and not BigInt directly?

I see, we don't have SpecBigInt yet...
We should add at some point, along with the necessary compiler changes.

[Saam Barati]

Comment on attachment 333056 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-945
>> -                    // profiling the wrong value, leading the ArithProfile to think it observed a non-number result.
> 
> Why remove this comment?

Ignore this. I see why you removed it.

[Saam Barati]

Comment on attachment 333056 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:201
> +            SpeculatedType prediction = node->child1()->prediction();
> +            if (prediction) {
> +                if (isInt32OrBooleanSpeculation(prediction) && node->canSpeculateInt32(m_pass))
> +                    changed |= mergePrediction(SpecInt32Only);
> +                else if (m_graph.unaryArithShouldSpeculateAnyInt(node, m_pass))
> +                    changed |= mergePrediction(SpecInt52Only);
> +                else if (isBytecodeNumberSpeculation(prediction))
> +                    changed |= mergePrediction(speculatedDoubleTypeForPrediction(node->child1()->prediction()));
> +                else {
> +                    changed |= mergePrediction(SpecInt32Only);
> +                    if (node->mayHaveNonNumberResult()) {
> +                        // FIXME: We should add support to BigInt into speculation
> +                        // https://bugs.webkit.org/show_bug.cgi?id=182470
> +                        changed |= mergePrediction(SpecCellOther);
> +                    }
> +                    if (node->mayHaveDoubleResult())
> +                        changed |= mergePrediction(SpecBytecodeDouble);
> +                }
> +            }
> +            break;

This code is almost identical to the ArithNegate case below. Let's combine them in some way and perhaps just branch on node->op() where they differ.

[Caio Lima]

Created attachment 335475 [details]
Patch to test on EWS

Testing in EWS. It includes code from https://bugs.webkit.org/show_bug.cgi?id=182470

[Caio Lima]

Created attachment 337703 [details]
Patch

[EWS Watchlist]

Comment on attachment 337703 [details]
Patch

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

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate

[EWS Watchlist]

Comment on attachment 337703 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 337718 [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 338072 [details]
Patch

[EWS Watchlist]

Comment on attachment 338072 [details]
Patch

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

New failing tests:
animations/needs-layout.html

[EWS Watchlist]

Created attachment 338075 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[Caio Lima]

Ping Review

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 338579 [details]
Patch

Patch rebased.

[Saam Barati]

Comment on attachment 338579 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:740
> +        forNode(node).setType(m_graph, SpecBytecodeNumber | SpecBigInt);

Is this only used for BigInt? If not, why else would it be used?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:255
> +    for (int i = 0; i < x->length(); i++)

should be unsigned

[Caio Lima]

Comment on attachment 338579 [details]
Patch

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

Thx for the review

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:740
>> +        forNode(node).setType(m_graph, SpecBytecodeNumber | SpecBigInt);
> 
> Is this only used for BigInt? If not, why else would it be used?

It is also used by Objects with valueOf or toString.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:255
>> +    for (int i = 0; i < x->length(); i++)
> 
> should be unsigned

Ok

[Caio Lima]

Created attachment 338956 [details]
Patch updated

Addressing Saam's comments.

[Caio Lima]

Created attachment 339233 [details]
Patch

[Caio Lima]

Created attachment 339241 [details]
Patch

[EWS Watchlist]

Comment on attachment 339241 [details]
Patch

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

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 339254 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Caio Lima]

Ping?

[Caio Lima]

Created attachment 339646 [details]
Patch

[Caio Lima]

Ping review

[Caio Lima]

Created attachment 340157 [details]
Patch

[Caio Lima]

Created attachment 340263 [details]
Patch

[EWS Watchlist]

Comment on attachment 340263 [details]
Patch

Attachment 340263 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7669225

New failing tests:
svg/custom/object-sizing-explicit-height.xhtml

[EWS Watchlist]

Created attachment 340265 [details]
Archive of layout-test-results from ews117 for mac-sierra

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

[Yusuke Suzuki]

Comment on attachment 340263 [details]
Patch

Could you ensure performance regression is not introduced by running benchmarks?

[Caio Lima]

(In reply to Yusuke Suzuki from comment #37)
> Comment on attachment 340263 [details]
> Patch
> 
> Could you ensure performance regression is not introduced by running
> benchmarks?

Sure!

[Caio Lima]

Created attachment 340270 [details]
Benchmark results

Here is the performance results. The patch is perf neutral in my machine.

[Yusuke Suzuki]

Comment on attachment 340263 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:766
> +        forNode(node).setType(m_graph, SpecBytecodeNumber | SpecBigInt);

Use setTypeForNode(node, SpecBytecodeNumber | SpecBigInt);

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:770
>      case ArithNegate: {

Should not have ArithNegate(Untyped) case. It is incorrect if the input is BigInt. It should be handled by ValueAdd. Let's add RELEASE_ASSERT_NOT_REACHED for "default" clause.

> Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:310
> -            
> +

Remove this change

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:163
> +            if (node->child1()->shouldSpeculateInt32OrBoolean() && node->canSpeculateInt32(FixupPass)) {
> +                node->setOp(ArithNegate);
> +                fixIntOrBooleanEdge(node->child1());
> +                if (bytecodeCanTruncateInteger(node->arithNodeFlags()))
> +                    node->setArithMode(Arith::Unchecked);
> +                else if (bytecodeCanIgnoreNegativeZero(node->arithNodeFlags()))
> +                    node->setArithMode(Arith::CheckOverflow);
> +                else
> +                    node->setArithMode(Arith::CheckOverflowAndNegativeZero);
> +                node->setResult(NodeResultInt32);
> +                node->clearFlags(NodeMustGenerate);
> +                break;
> +            }

Nice.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:186
> +        }

And let's fix ArithNegate case too. ArithNegate can produce ArithNegate(Untyped). But it is wrong... ArithNegate(BigInt) would produce BigInt, but ArithNegate node type claims that it always produces numbers.
So, we should remove ArithNegate(Untyped) case, and this should be handled ValueNegate(Untyped). This is the same strategy to ArithAdd and ValueAdd.
We should fixup edges with some number filters in ArithNegate. If int fixup (Int32, Int52 etc.) is failed in ArithNegate, we should always call fixDoubleOrBooleanEdge(node->child1()) to emit edge filter.
This is OK since bytecode parser emits ArithNegate only for operands which result is number. Original ArithNegate(Untyped) thing should be done by ValueNegate.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:258
>                  else {

We should have BigInt speculation type, edge filter, and speculation check here. Let's add FIXME and url.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4533
> +    return;

This "return" is unnecessary.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4536
>  void SpeculativeJIT::compileArithNegate(Node* node)

Why does ArithNegate have ArithNegate(Untyped)? Can we use ValueNegate for that case?
Since ArithNegate's result type should be number, we cannot take BigInt for that. So, current ArithNegate(Untyped) is wrong with BigInt.

ArithAdd does not have ArithAdd(Untyped, Untyped). Instead it uses ValueAdd(Untyped, Untyped) for that. Since ArithNegate is emitted in bytecode parser if the operand has NumberResult, and we only convert ValueAdd to ArithAdd with number related edge filtering, we can remove ArithNegate(Untyped) case if we carefully change fixup etc.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2720
>      void compileArithNegate()

Ditto. Remove ArithNegate(Untyped) case. And please ensure ArithAdd(Untyped) is not produced.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:253
> +JSBigInt* JSBigInt::copy(ExecState* state, JSBigInt* x)

Take VM& instead of ExecState*.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:264
> +JSBigInt* JSBigInt::unaryMinus(ExecState* state, JSBigInt* x)

Take VM& instead of ExecState*.

> Source/JavaScriptCore/runtime/JSBigInt.h:81
> +    enum ParseIntMode { DisallowEmptyString, AllowEmptyString };
> +    enum ParseIntSign { Unsigned, Signed };

Use enum class.

[Caio Lima]

Created attachment 340808 [details]
Patch

[Caio Lima]

Comment on attachment 340263 [details]
Patch

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

Thanks for the Review!

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:186
>> +        }
> 
> And let's fix ArithNegate case too. ArithNegate can produce ArithNegate(Untyped). But it is wrong... ArithNegate(BigInt) would produce BigInt, but ArithNegate node type claims that it always produces numbers.
> So, we should remove ArithNegate(Untyped) case, and this should be handled ValueNegate(Untyped). This is the same strategy to ArithAdd and ValueAdd.
> We should fixup edges with some number filters in ArithNegate. If int fixup (Int32, Int52 etc.) is failed in ArithNegate, we should always call fixDoubleOrBooleanEdge(node->child1()) to emit edge filter.
> This is OK since bytecode parser emits ArithNegate only for operands which result is number. Original ArithNegate(Untyped) thing should be done by ValueNegate.

Cool! You are right.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4536
>>  void SpeculativeJIT::compileArithNegate(Node* node)
> 
> Why does ArithNegate have ArithNegate(Untyped)? Can we use ValueNegate for that case?
> Since ArithNegate's result type should be number, we cannot take BigInt for that. So, current ArithNegate(Untyped) is wrong with BigInt.
> 
> ArithAdd does not have ArithAdd(Untyped, Untyped). Instead it uses ValueAdd(Untyped, Untyped) for that. Since ArithNegate is emitted in bytecode parser if the operand has NumberResult, and we only convert ValueAdd to ArithAdd with number related edge filtering, we can remove ArithNegate(Untyped) case if we carefully change fixup etc.

Sure. Removing it.

[Caio Lima]

Created attachment 340812 [details]
Benchmark Report

This is the new result of benchmarks. These changes are perf neutral.

[Caio Lima]

Ping Review

[Caio Lima]

Ping?

[Yusuke Suzuki]

Comment on attachment 340808 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/jit/JITOperations.cpp:2720
> +        return JSValue::encode(JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue))));

Do not need `JSValue()` wrap around JSBigInt result. `JSValue::encode(JSBigInt::xxx())` works.

> Source/JavaScriptCore/jit/JITOperations.cpp:2740
> +        JSValue result = JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue)));

Do not need this `JSValue()` wrap. Just,

JSBigInt* result = JSBigInt::xxx();
arithProfile.observeRusult(result);

works.

> Source/JavaScriptCore/jit/JITOperations.cpp:2785
> +        JSValue result = JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue)));

Ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:2817
> +        JSValue result = JSValue(JSBigInt::unaryMinus(vm, asBigInt(primValue)));

Ditto.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:406
> +        JSValue result = JSValue(JSBigInt::unaryMinus(exec->vm(), asBigInt(primValue)));

Ditto.

[Caio Lima]

Created attachment 341279 [details]
Patch For landing

Let's see EWS

[Dominik Inführ]

Did you link the wrong bugzilla id in `// https://bugs.webkit.org/show_bug.cgi?id=18247`? It points to "On Mac: showModalDialog doesn't follow Apple's Human Interface Guidelines" right now.

[Caio Lima]

(In reply to Dominik Inführ from comment #48)
> Did you link the wrong bugzilla id in `//
> https://bugs.webkit.org/show_bug.cgi?id=18247`? It points to "On Mac:
> showModalDialog doesn't follow Apple's Human Interface Guidelines" right now.

Nice Catch. I'm going to update it.

[Caio Lima]

Created attachment 341440 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 341440 [details]
Patch for landing

Clearing flags on attachment: 341440

Committed r232232: <https://trac.webkit.org/changeset/232232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40590283>