Bug 190923

Summary:

Re-introduce op_bitnot

Product:

WebKit

Reporter:

Caio Lima <ticaiolima>

Component:

JavaScriptCore

Assignee:

Caio Lima <ticaiolima>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, fpizlo, guijemont, guijemont+jsc-armv7-ews, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

182216

Attachments:

Description	Flags
WIP - Patch	none
Archive of layout-test-results from ews201 for win-future	none
WIP - Patch	none
Archive of layout-test-results from ews203 for win-future	none
Archive of layout-test-results from ews202 for win-future	none
EWS win test	none
Archive of layout-test-results from ews205 for win-future	none
Archive of layout-test-results from ews204 for win-future	none
Patch	none
Patch to test ews	none
Patch	none
Patch	none
Archive of layout-test-results from ews201 for win-future	none
Patch	none
Archive of layout-test-results from ews122 for ios-simulator-wk2	none
Patch	none
Patch for news	none
Archive of layout-test-results from ews202 for win-future	none
Patch to test ews	none
WIP - Patch	none
WIP - Patch	none
WIP - Patch	none
Patch	none
Patch	none
Benchmarks	none
Patch	none

Description Caio Lima 2018-10-25 15:48:27 PDT

With the introduction of BigInt, emitting BitNotNode as x ^ -1 is not safe anymore. This way, we need to re-introduce op_bitnot.

Comment 1 Caio Lima 2018-11-14 10:36:04 PST

Created attachment 354831 [details]
WIP - Patch

Comment 2 EWS Watchlist 2018-11-14 12:35:26 PST

Comment on attachment 354831 [details]
WIP - Patch

Attachment 354831 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9992336

New failing tests:
js/kde/md5-2.html

Comment 3 EWS Watchlist 2018-11-14 12:35:39 PST

Created attachment 354843 [details]
Archive of layout-test-results from ews201 for win-future

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

Comment 4 Caio Lima 2018-11-14 18:00:12 PST

Created attachment 354881 [details]
WIP - Patch

Comment 5 EWS Watchlist 2018-11-14 20:53:02 PST

Comment on attachment 354881 [details]
WIP - Patch

Attachment 354881 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9998041

New failing tests:
js/kde/md5-2.html

Comment 6 EWS Watchlist 2018-11-14 20:53:13 PST

Created attachment 354887 [details]
Archive of layout-test-results from ews203 for win-future

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

Comment 7 Yusuke Suzuki 2018-11-14 21:35:38 PST

Comment on attachment 354881 [details]
WIP - Patch

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

I think this direction is good. Some nits before the review.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:1053
> +    void not32(RegisterID srcDest, RegisterID)

The second argument is not necessary.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3539
> +    m_jit.not32(resultGPR, resultGPR);

Use `not32(resultGPR);`.

Comment 8 EWS Watchlist 2018-11-14 22:43:12 PST

Comment on attachment 354881 [details]
WIP - Patch

Attachment 354881 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9998659

New failing tests:
js/kde/md5-2.html

Comment 9 EWS Watchlist 2018-11-14 22:43:24 PST

Created attachment 354891 [details]
Archive of layout-test-results from ews202 for win-future

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

Comment 10 Caio Lima 2018-11-15 07:54:27 PST

Created attachment 354933 [details]
EWS win test

Comment 11 EWS Watchlist 2018-11-15 10:02:14 PST

Comment on attachment 354933 [details]
EWS win test

Attachment 354933 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10004727

New failing tests:
js/kde/md5-2.html

Comment 12 EWS Watchlist 2018-11-15 10:02:25 PST

Created attachment 354952 [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 13 EWS Watchlist 2018-11-15 11:45:48 PST

Comment on attachment 354933 [details]
EWS win test

Attachment 354933 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10005721

New failing tests:
js/kde/md5-2.html

Comment 14 EWS Watchlist 2018-11-15 11:45:59 PST

Created attachment 354964 [details]
Archive of layout-test-results from ews204 for win-future

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

Comment 15 Caio Lima 2018-11-15 14:31:02 PST

Created attachment 354989 [details]
Patch

Comment 16 Caio Lima 2018-11-15 14:47:59 PST

Created attachment 354991 [details]
Patch to test ews

Comment 17 Caio Lima 2018-11-16 05:09:03 PST

Created attachment 355043 [details]
Patch

Comment 18 Caio Lima 2018-11-16 15:28:01 PST

Created attachment 355141 [details]
Patch

Comment 19 EWS Watchlist 2018-11-16 19:30:39 PST

Comment on attachment 355141 [details]
Patch

Attachment 355141 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10027970

New failing tests:
sputnik/Conformance/11_Expressions/11.4_Unary_Operators/11.4.8_Bitwise_NOT_Operator/S11.4.8_A2.1_T1.html
js/kde/md5-2.html

Comment 20 EWS Watchlist 2018-11-16 19:30:50 PST

Created attachment 355171 [details]
Archive of layout-test-results from ews201 for win-future

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

Comment 21 Caio Lima 2018-11-16 20:13:48 PST

Created attachment 355175 [details]
Patch

Comment 22 EWS Watchlist 2018-11-16 23:01:24 PST

Comment on attachment 355175 [details]
Patch

Attachment 355175 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10029681

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html

Comment 23 EWS Watchlist 2018-11-16 23:01:26 PST

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

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

Comment 24 Caio Lima 2018-11-17 13:13:13 PST

Created attachment 355198 [details]
Patch

Comment 25 Caio Lima 2018-11-17 14:03:48 PST

Created attachment 355203 [details]
Patch for news

Comment 26 EWS Watchlist 2018-11-17 16:00:27 PST

Comment on attachment 355203 [details]
Patch for news

Attachment 355203 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10047326

New failing tests:
sputnik/Conformance/11_Expressions/11.4_Unary_Operators/11.4.8_Bitwise_NOT_Operator/S11.4.8_A2.1_T1.html
js/kde/md5-2.html

Comment 27 EWS Watchlist 2018-11-17 16:00:38 PST

Created attachment 355212 [details]
Archive of layout-test-results from ews202 for win-future

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

Comment 28 Caio Lima 2018-11-17 17:59:56 PST

Created attachment 355216 [details]
Patch to test ews

Comment 29 Caio Lima 2018-11-18 15:55:10 PST

Created attachment 355241 [details]
WIP - Patch

Comment 30 Caio Lima 2018-11-18 15:55:58 PST

Comment on attachment 355241 [details]
WIP - Patch

Rebasing with master

Comment 31 Caio Lima 2018-11-19 06:35:49 PST

Created attachment 355277 [details]
WIP - Patch

Comment 32 Caio Lima 2018-11-19 07:29:29 PST

Created attachment 355280 [details]
WIP - Patch

Comment 33 Caio Lima 2018-11-21 11:08:34 PST

Created attachment 355430 [details]
Patch

Comment 34 Caio Lima 2018-11-26 04:21:34 PST

Created attachment 355636 [details]
Patch

Comment 35 Yusuke Suzuki 2018-11-26 06:16:27 PST

Comment on attachment 355636 [details]
Patch

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

r=me with nits. Please ensure it does not cause any performance regression before landing.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:388
> +            setConstant(node, JSValue(~a));

Insert `break;` here.

> Source/JavaScriptCore/dfg/DFGNodeType.h:114
> +    macro(ArithBitNot, NodeResultInt32) \

Add `NodeMustGenerate` since the Untyped case can cause anything.
And clear `NodeMustGenerate` when the edge is fixed by `Int32` in the fixup phase.
Later, if you add ValueBitNot, then you can make this non-MustGenerate, and handle Untyped cases in ValueBitNot.

Comment 36 Caio Lima 2018-11-26 16:06:43 PST

Created attachment 355689 [details]
Benchmarks

The Patch is perf neutral on x86_64 macOS.

Comment 37 Caio Lima 2018-11-26 17:41:43 PST

Created attachment 355698 [details]
Patch

Comment 38 Caio Lima 2018-11-26 17:43:09 PST

Thank you very much for the review!

(In reply to Yusuke Suzuki from comment #35)
> Comment on attachment 355636 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355636&action=review
> 
> r=me with nits. Please ensure it does not cause any performance regression
> before landing.
> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:388
> > +            setConstant(node, JSValue(~a));
> 
> Insert `break;` here.

Done!

> > Source/JavaScriptCore/dfg/DFGNodeType.h:114
> > +    macro(ArithBitNot, NodeResultInt32) \
> 
> Add `NodeMustGenerate` since the Untyped case can cause anything.
> And clear `NodeMustGenerate` when the edge is fixed by `Int32` in the fixup
> phase.
> Later, if you add ValueBitNot, then you can make this non-MustGenerate, and
> handle Untyped cases in ValueBitNot.

Done. I also added a test case. thx for catching this.

Comment 39 WebKit Commit Bot 2018-11-26 18:26:06 PST

The commit-queue encountered the following flaky tests while processing attachment 355698 [details]:

webgl/2.0.0/conformance2/rendering/blitframebuffer-filter-outofbounds.html bug 191991 (author: justin_fan@apple.com)
webgl/2.0.0/conformance/more/functions/readPixels.html bug 191992 (author: justin_fan@apple.com)
The commit-queue is continuing to process your patch.

Comment 40 WebKit Commit Bot 2018-11-26 18:27:05 PST

Comment on attachment 355698 [details]
Patch

Clearing flags on attachment: 355698

Committed r238543: <https://trac.webkit.org/changeset/238543>

Comment 41 WebKit Commit Bot 2018-11-26 18:27:08 PST

All reviewed patches have been landed.  Closing bug.

Comment 42 Radar WebKit Bug Importer 2018-11-26 18:28:55 PST

<rdar://problem/46264064>

Comment 43 Guillaume Emont 2018-11-29 10:10:45 PST

r238543 introduced a regression on Armv7. See Bug 192152.

Comment 44 Saam Barati 2018-11-29 18:00:29 PST

Comment on attachment 355698 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:390
> +        JSValue operand = forNode(node->child1()).value();
> +        if (operand && operand.isInt32()) {
> +            int32_t a = operand.asInt32();
> +            setConstant(node, JSValue(~a));
> +            break;
> +        }

This constant folding can happen irrespective of being UntypedUse or Int32Use

Comment 45 Saam Barati 2018-11-29 18:07:59 PST

Comment on attachment 355698 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855
> +            LValue result = vmCall(pointerType(), m_out.operation(operationValueBitNot), m_callFrame, operand);

nit: Should be Int64 not pointerType()

Comment 46 Caio Lima 2018-11-30 12:43:55 PST

(In reply to Saam Barati from comment #45)
> Comment on attachment 355698 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355698&action=review
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855
> > +            LValue result = vmCall(pointerType(), m_out.operation(operationValueBitNot), m_callFrame, operand);
> 
> nit: Should be Int64 not pointerType()

I will handle both comments into https://bugs.webkit.org/show_bug.cgi?id=182216

Comment 47 Caio Lima 2018-11-30 12:44:43 PST

(In reply to Guillaume Emont from comment #43)
> r238543 introduced a regression on Armv7. See Bug 192152.

This probably will be fixed  by https://bugs.webkit.org/show_bug.cgi?id=192203