Bug 186228 - [ESNext][BigInt] Implement support for "&"
Summary: [ESNext][BigInt] Implement support for "&"
Status: NEW
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:
Depends on:
Blocks: 179001
  Show dependency treegraph
 
Reported: 2018-06-02 06:49 PDT by Caio Lima
Modified: 2018-07-08 23:34 PDT (History)
9 users (show)

See Also:


Attachments
Patch (30.89 KB, patch)
2018-06-03 11:44 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.78 MB, application/zip)
2018-06-03 16:21 PDT, Build Bot
no flags Details
Patch (48.29 KB, patch)
2018-06-07 20:54 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmarks Report (95.10 KB, text/plain)
2018-06-07 21:00 PDT, Caio Lima
no flags Details
Patch (48.44 KB, patch)
2018-06-07 21:09 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.77 MB, application/zip)
2018-06-07 23:42 PDT, Build Bot
no flags Details
Patch (52.14 KB, patch)
2018-06-08 07:45 PDT, Caio Lima
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (5.20 MB, application/zip)
2018-06-08 09:47 PDT, Build Bot
no flags Details
Patch (48.99 KB, patch)
2018-07-04 15:06 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (51.09 KB, patch)
2018-07-08 23:30 PDT, Caio Lima
ticaiolima: review?
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-06-02 06:49:29 PDT
...
Comment 1 Caio Lima 2018-06-03 11:44:55 PDT
Created attachment 341869 [details]
Patch
Comment 2 Build Bot 2018-06-03 16:21:21 PDT
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
Comment 3 Build Bot 2018-06-03 16:21:33 PDT
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
Comment 4 Yusuke Suzuki 2018-06-03 19:32:42 PDT
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).
Comment 5 Caio Lima 2018-06-07 20:54:20 PDT
Created attachment 342234 [details]
Patch
Comment 6 Caio Lima 2018-06-07 20:58:26 PDT
(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.
Comment 7 Caio Lima 2018-06-07 21:00:25 PDT
Created attachment 342235 [details]
Benchmarks Report

Based on benchmark results in my machine, this Patch is perf neutral.
Comment 8 Caio Lima 2018-06-07 21:09:29 PDT
Created attachment 342236 [details]
Patch
Comment 9 Build Bot 2018-06-07 23:10:15 PDT
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.
Comment 10 Build Bot 2018-06-07 23:41:58 PDT
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
Comment 11 Build Bot 2018-06-07 23:42:09 PDT
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
Comment 12 Yusuke Suzuki 2018-06-08 03:07:25 PDT
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.
Comment 13 Caio Lima 2018-06-08 05:23:29 PDT
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.
Comment 14 Caio Lima 2018-06-08 07:45:39 PDT
Created attachment 342259 [details]
Patch
Comment 15 Caio Lima 2018-06-08 07:46:35 PDT
This version is adding the BigInt speculation code generation into DFG and FTL.
Comment 16 Build Bot 2018-06-08 07:47:43 PDT
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.
Comment 17 Build Bot 2018-06-08 09:03:29 PDT
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
Comment 18 Build Bot 2018-06-08 09:47:43 PDT
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
Comment 19 Build Bot 2018-06-08 09:47:45 PDT
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
Comment 20 Caio Lima 2018-06-14 07:26:10 PDT
Ping
Comment 21 Caio Lima 2018-06-16 07:10:06 PDT
Ping Review
Comment 22 Caio Lima 2018-06-23 13:26:56 PDT
Ping Review
Comment 23 Caio Lima 2018-07-01 23:10:06 PDT
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?
Comment 24 Caio Lima 2018-07-04 15:06:41 PDT
Created attachment 344302 [details]
Patch

Rebasing Patch.
Comment 25 Build Bot 2018-07-04 15:09:13 PDT
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.
Comment 26 Caio Lima 2018-07-08 13:56:18 PDT
Ping
Comment 27 Caio Lima 2018-07-08 23:30:59 PDT
Created attachment 344566 [details]
Patch
Comment 28 Build Bot 2018-07-08 23:34:08 PDT
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.