Bug 186228 – [ESNext][BigInt] Implement support for "&"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

...

[Caio Lima]

Created attachment 341869 [details]
Patch

[EWS Watchlist]

Comment on attachment 341869 [details]
Patch

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

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 341875 [details]
Archive of layout-test-results from ews205 for win-future

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

[Yusuke Suzuki]

Comment on attachment 341869 [details]
Patch

Previously, op_bitand never returns value except for Int32. But this assumption is broken now. We should do arith-profile things well to emit BitAnd effectively in DFG.
This means we should profile values (result, and operands) carefully in LLInt and Baseline as the other arithmetic operations do.
And please measure the performance once it is done :)

And I think we should rename DFG bit op nodes from BitAnd to ArithBitAnd since later we will add ValueBitAnd. (And we should rename other bit nodes too).

[Caio Lima]

Created attachment 342234 [details]
Patch

[Caio Lima]

(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 341869 [details]
> Patch
> 
> Previously, op_bitand never returns value except for Int32. But this
> assumption is broken now. We should do arith-profile things well to emit
> BitAnd effectively in DFG.
> This means we should profile values (result, and operands) carefully in
> LLInt and Baseline as the other arithmetic operations do.
> And please measure the performance once it is done :)
> 
> And I think we should rename DFG bit op nodes from BitAnd to ArithBitAnd
> since later we will add ValueBitAnd. (And we should rename other bit nodes
> too).

Cool. I decided to add ValueProfile to op_bitand after talking with Saam. We are profiling operands in op_add, op_sub, etc, because we have IC for such cases. But I don't think there is a necessity to add IC for op_bitand now and on DFG and FTL we already can get operands type prediction with the prediction propagation. Also, I'm just profiling when the result is BigInt, otherwise, it needs to be Int32. Unfortunately, it made the Patch way more big to be reviewed.

[Caio Lima]

Created attachment 342235 [details]
Benchmarks Report

Based on benchmark results in my machine, this Patch is perf neutral.

[Caio Lima]

Created attachment 342236 [details]
Patch

[EWS Watchlist]

Attachment 342236 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 342236 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 342243 [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

[Yusuke Suzuki]

Comment on attachment 342236 [details]
Patch

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

After looking the current implementation, I think we should show the goal of DFG / FTL implementation first before starting the implementation of bit operations, since it may involve how to profile things effectively.
I think completing DFG / FTL implementations for binary operations (like add, sub etc.) first is better to show the goal of the design and implementation for binary operations with BigInt.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1747
> +    if (opcodeID == op_bitand) {
> +        UnlinkedValueProfile profile = emitProfiledOpcode(opcodeID);
> +        instructions().append(dst->index());
> +        instructions().append(src1->index());
> +        instructions().append(src2->index());
> +        instructions().append(profile);
> +        return dst;
> +    }

Why not using ArithProfile, which is already emitted in the existing code?
To clarify whether ArithProfile is enough, I think completing add / sub etc. for BigInt in DFG and FTL first is better.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1756
>          opcodeID == op_add || opcodeID == op_mul || opcodeID == op_sub || opcodeID == op_div)
>          instructions().append(ArithProfile(types.first(), types.second()).bits());

We already emit ArithProfile for op_bitand. I think what we should do is record necessary information in this profile for op_bitand.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4641
> +            if (getPredictionWithoutOSRExit() != SpecBigInt)
> +                set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ArithBitAnd, op1, op2));
> +            else
> +                set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ValueBitAnd, op1, op2));
> +

I think relying on ArithProfile is better since it is the same to the other binary operations including op_sub.

[Caio Lima]

Thank you for the feedback.

(In reply to Yusuke Suzuki from comment #12)
> Comment on attachment 342236 [details]
> Patch
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1747
> > +    if (opcodeID == op_bitand) {
> > +        UnlinkedValueProfile profile = emitProfiledOpcode(opcodeID);
> > +        instructions().append(dst->index());
> > +        instructions().append(src1->index());
> > +        instructions().append(src2->index());
> > +        instructions().append(profile);
> > +        return dst;
> > +    }
> 
> Why not using ArithProfile, which is already emitted in the existing code?
> To clarify whether ArithProfile is enough, I think completing add / sub etc.
> for BigInt in DFG and FTL first is better.

ArithProfile is much more complex to this use case IMO. While op_add or op_sub profiles their operands for IC and also profile the result to check overflow or negative zero, bitwise operations never have these outcomes. Introducing BigInt will only make bitwise operations return In32 or BigInt, and to return BigInt, both operands need to be be BigInt after ToNumeric operation, which restricts the possibility of operands to be JSCell (but not string or symbol) or BigInt. With ValueProfile we already are able to get the result type as feedback.

Maybe I'm missing something here, but my idea is to specialize these nodes for BigInt into DFG and FTL calling slow paths, in the first moment, and then emmiting code that is able to unbox BigInts and perform operations directly in binary. If I'm not wrong, this requires the type information of operands that is already accessible on DFG and FTL. It is not clear to me why implement op_sub first is more important the bitwise operations. Do you mind explain why we should follow this path?

> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4641
> > +            if (getPredictionWithoutOSRExit() != SpecBigInt)
> > +                set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ArithBitAnd, op1, op2));
> > +            else
> > +                set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(ValueBitAnd, op1, op2));
> > +
> 
> I think relying on ArithProfile is better since it is the same to the other
> binary operations including op_sub.

I mentioned above why I think ValueProfile make more sense here. AFAIC, op_sub is just emitting ArithSub right now. However, we have ArithNegate and ArithAdd that decides to emit ValueAdd or AirthAdd depending of operands type. However, they doesn't use ArithProfile on "DFGByteCodeParser.cpp". They rely on each operand's node type to emit proper node.

[Caio Lima]

Created attachment 342259 [details]
Patch

[Caio Lima]

This version is adding the BigInt speculation code generation into DFG and FTL.

[EWS Watchlist]

Attachment 342259 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 342259 [details]
Patch

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

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

[EWS Watchlist]

Comment on attachment 342259 [details]
Patch

Attachment 342259 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8082131

New failing tests:
http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-with-partitioning-timeout.html

[EWS Watchlist]

Created attachment 342278 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Caio Lima]

Ping

[Caio Lima]

Ping Review

[Caio Lima]

Ping Review

[Caio Lima]

Ping Review here.

I also started the implementation of ValueSub into https://bugs.webkit.org/show_bug.cgi?id=186176. I think the last review Yusuke mentioned that we should implement such support before, but comparing this patch with ValueBitAnd JIT implementation, they does not see related.

@Yusuke, What do you think about it?

[Caio Lima]

Created attachment 344302 [details]
Patch

Rebasing Patch.

[EWS Watchlist]

Attachment 344302 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Ping

[Caio Lima]

Created attachment 344566 [details]
Patch

[EWS Watchlist]

Attachment 344566 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:424:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 350526 [details]
Patch

Patch rebased

[EWS Watchlist]

Attachment 350526 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 350532 [details]
Patch

[EWS Watchlist]

Attachment 350532 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Ping review

[Yusuke Suzuki]

Comment on attachment 350532 [details]
Patch

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

Looks nice. I would like to have one more round of reviews.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848
> +            if (getPredictionWithoutOSRExit() != SpecBigInt)

If `type = SpecBigInt | SpecInt32Only`, it goes to `ArithBitAnd`. It is not desirable result.
We should use `isInt32Speculation(getPredictionWithoutOSRExit())` => ArithBitAnd.

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

Both operands use `child1()`.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:121
> +            if (Node::shouldSpeculateUntypedForBitOps(node->child1().node(), node->child2().node())) {
> +                fixEdge<UntypedUse>(node->child1());
> +                fixEdge<UntypedUse>(node->child2());
> +                node->setOp(ValueBitAnd);
> +                node->setResult(NodeResultJS);
> +                break;
> +            }

If the type is `ValueBitAnd`, it should be `NodeMustBeGenerate`. You need to mark it as `NodeMustBeGenerate`. You can use `setOpAndDefaultFlags(ValueBitAnd)` instead of setOp(ValueBitAnd) and setResult(NodeResultJS);.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3613
> +    GPRTemporary result(this);

Remove this.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3626
> +    silentSpillAllRegisters(resultGPR);
> +    callOperation(operationBitAndBigInt, resultGPR, leftGPR, rightGPR);
> +    silentFillAllRegisters(); 
> +    m_jit.exceptionCheck();
> +
> +    cellResult(resultGPR, node);

In such case, you can use,

flushRegisters();
GPRFlushedCallResult result(this);
GPRReg resultGPR = result.gpr();
callOperation(operationBitAndBigInt, resultGPR, leftGPR, rightGPR);
m_jit.exceptionCheck();
cellResult(resultGPR, node);

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:655
> +        case DFG::ArithBitAnd:

Can we just use `ArithBitAnd` here? (b/c it does not conflict with B3 BitAnd).

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

Remove this line.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:98
> +    bigInt->setSign(false);

This should be done in JSBigInt::JSBigInt constructor.

, m_sign(false)

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:756
> +    if (isInt32())
> +        return asInt32();

What happens if the `this` is double and it is in range of int32? (e.g. 1.0).
I think we can have `if (isDouble())` path too. `canBeInt32` function can be used for this case.

[Yusuke Suzuki]

Comment on attachment 350532 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:61
> +        * llint/LowLevelInterpreter32_64.asm:

Let's add LLInt change. It is required since now op_bitand starts using ValueProfile instead of ArithProfile.

[Yusuke Suzuki]

Comment on attachment 350532 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:61
>> +        * llint/LowLevelInterpreter32_64.asm:
> 
> Let's add LLInt change. It is required since now op_bitand starts using ValueProfile instead of ArithProfile.

And we also need Baseline JIT Change for ValueProfile.

[Caio Lima]

Comment on attachment 350532 [details]
Patch

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

Thank you very much for the review.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848
>> +            if (getPredictionWithoutOSRExit() != SpecBigInt)
> 
> If `type = SpecBigInt | SpecInt32Only`, it goes to `ArithBitAnd`. It is not desirable result.
> We should use `isInt32Speculation(getPredictionWithoutOSRExit())` => ArithBitAnd.

Hum...That's a good edge case:

```
function foo(a, b) {
    return  a & b
}
noInline(foo);

for (let i = 0; i < 10000; i++) {
    if (i % 2) {
        foo(3, 4);
    } else {
       foo(3n, 4n);
    }
}
```
Right now, we only profile BigInt results, which means that the profile will say that only BigInt was observed as outcome of "op_bitand", but it is not true.

I will change that.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:103
>> +            if (Node::shouldSpeculateBigInt(node->child1().node(), node->child1().node())) {
> 
> Both operands use `child1()`.

Oops

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:121
>> +            }
> 
> If the type is `ValueBitAnd`, it should be `NodeMustBeGenerate`. You need to mark it as `NodeMustBeGenerate`. You can use `setOpAndDefaultFlags(ValueBitAnd)` instead of setOp(ValueBitAnd) and setResult(NodeResultJS);.

Changed.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:756
>> +        return asInt32();
> 
> What happens if the `this` is double and it is in range of int32? (e.g. 1.0).
> I think we can have `if (isDouble())` path too. `canBeInt32` function can be used for this case.

1.0 goes through ```isInt32()``` path. However, 223.0 fallback to slow path. I'm adding the case when we have double that canBeInt32.

[Caio Lima]

Created attachment 351082 [details]
Patch

[EWS Watchlist]

Attachment 351082 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 351082 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848
> +            if (isInt32Speculation(getPredictionWithoutOSRExit()))

Use `getPrediction()`. We always profile the value appropriately.

> Source/JavaScriptCore/jit/JIT.h:797
> +        void emitBitBinaryOpFastPath(Instruction* currentInstruction, bool shouldEmitProfiling = false);

Let's have `enum class` for profiling mode instead of `bool`.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1162
> +    valueProfile(t0, t3, 16, t2)

valueProfile's argument seems wrong. `valueProfile(tag, payload, operand, scratch)`.
And can we make `16` `((advance - 1) * 4)`?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1106
> +    valueProfile(t0, 4, t2)

Can we use `(advance - 1)` instead of `4`?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:112
>      } else {
>          bigInt->setDigit(0, static_cast<Digit>(value));
> -        bigInt->setSign(false);
>      }

Remove `{` and `}`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:140
>          } else {
>              bigInt->setDigit(0, static_cast<Digit>(value));
> -            bigInt->setSign(false);
>          }

Remove `{` and `}`.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:758
> +    if (isDouble() && canBeInt32(asDouble()))
> +        return JSC::toInt32(asDouble());

I think `return static_cast<int32_t>(asDouble());` is OK since canBeInt32 goes well.
Other cases go to the generic path below.

[Caio Lima]

Created attachment 351167 [details]
Patch

[Caio Lima]

Comment on attachment 351167 [details]
Patch

Thank you very much for the review @Yusuke!

[EWS Watchlist]

Attachment 351167 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:426:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 351167 [details]
Patch

Clearing flags on attachment: 351167

Committed r236637: <https://trac.webkit.org/changeset/236637>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44882601>

[Caio Lima]

Created attachment 351214 [details]
benchmarks

Patch is perf neutral.