Bug 190923 – Re-introduce op_bitnot

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

[Caio Lima]

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

[Caio Lima]

Created attachment 354831 [details]
WIP - Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 354881 [details]
WIP - Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

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);`.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 354933 [details]
EWS win test

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 354989 [details]
Patch

[Caio Lima]

Created attachment 354991 [details]
Patch to test ews

[Caio Lima]

Created attachment 355043 [details]
Patch

[Caio Lima]

Created attachment 355141 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 355175 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 355198 [details]
Patch

[Caio Lima]

Created attachment 355203 [details]
Patch for news

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caio Lima]

Created attachment 355216 [details]
Patch to test ews

[Caio Lima]

Created attachment 355241 [details]
WIP - Patch

[Caio Lima]

Comment on attachment 355241 [details]
WIP - Patch

Rebasing with master

[Caio Lima]

Created attachment 355277 [details]
WIP - Patch

[Caio Lima]

Created attachment 355280 [details]
WIP - Patch

[Caio Lima]

Created attachment 355430 [details]
Patch

[Caio Lima]

Created attachment 355636 [details]
Patch

[Yusuke Suzuki]

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.

[Caio Lima]

Created attachment 355689 [details]
Benchmarks

The Patch is perf neutral on x86_64 macOS.

[Caio Lima]

Created attachment 355698 [details]
Patch

[Caio Lima]

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.

[WebKit Commit Bot]

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: [e-mail])
webgl/2.0.0/conformance/more/functions/readPixels.html bug 191992 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 355698 [details]
Patch

Clearing flags on attachment: 355698

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46264064>

[Guillaume Emont]

r238543 introduced a regression on Armv7. See Bug 192152.

[Saam Barati]

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

[Saam Barati]

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

[Caio Lima]

(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

[Caio Lima]

(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