Bug 192723

Summary:

[BigInt] We should enable CSE into arithmetic operations that speculate BigIntUse

Product:

WebKit

Reporter:

Caio Lima <ticaiolima>

Component:

JavaScriptCore

Assignee:

Caio Lima <ticaiolima>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, ryanhaddad, saam, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

192921

Bug Blocks:

Attachments:

Description	Flags
Proposed Patch	none
Patch	none

Caio Lima

Reported 2018-12-14 16:16:03 PST

Now, our rules for ValueArith nodes are conservative and we don't allow CSE. In the case of SpecBigInt, we can be less conservative and allow CSE.

Attachments
Proposed Patch (13.07 KB, patch) 2018-12-17 05:51 PST, Caio Lima	no flags	Details Formatted Diff Diff
Patch (13.12 KB, patch) 2018-12-20 02:37 PST, Caio Lima	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Caio Lima

Comment 1 2018-12-17 05:51:11 PST

Created attachment 357436 [details] Proposed Patch

Saam Barati

Comment 2 2018-12-18 23:33:14 PST

Comment on attachment 357436 [details] Proposed Patch r=me

Caio Lima

Comment 3 2018-12-19 03:47:00 PST

Comment on attachment 357436 [details] Proposed Patch Thank you very much for the review.

WebKit Commit Bot

Comment 4 2018-12-19 04:12:45 PST

Comment on attachment 357436 [details] Proposed Patch Clearing flags on attachment: 357436 Committed r239377: <https://trac.webkit.org/changeset/239377>

WebKit Commit Bot

Comment 5 2018-12-19 04:12:47 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 6 2018-12-19 04:13:38 PST

<rdar://problem/46838118>

Ryan Haddad

Comment 7 2018-12-19 17:39:14 PST

It looks like this change caused 35 JSC tests to fail an assertion: ASSERTION FAILED: node() /Volumes/Data/slave/highsierra-debug/build/Source/JavaScriptCore/dfg/DFGEdge.h(82) : JSC::DFG::UseKind JSC::DFG::Edge::useKind() const 1 0x101610ec9 WTFCrash 2 0x101611ebb WTFCrashWithInfo(int, char const*, char const*, int) 3 0x10175245d JSC::DFG::Edge::useKind() const 4 0x10176ff95 JSC::DFG::Node::binaryUseKind() 5 0x102507d39 JSC::DFG::StrengthReductionPhase::handleNode() 6 0x10250656f JSC::DFG::StrengthReductionPhase::run() 7 0x102506351 bool JSC::DFG::runAndLog<JSC::DFG::StrengthReductionPhase>(JSC::DFG::StrengthReductionPhase&) 8 0x1024fe72e bool JSC::DFG::runPhase<JSC::DFG::StrengthReductionPhase>(JSC::DFG::Graph&) 9 0x1024fe6f5 JSC::DFG::performStrengthReduction(JSC::DFG::Graph&) 10 0x10249a9d7 JSC::DFG::Plan::compileInThreadImpl() 11 0x102499112 JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*) 12 0x1022e21fe JSC::DFG::compileImpl(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue> const&, WTF::Ref<JSC::DeferredCompilationCallback, WTF::DumbPtrTraits<JSC::DeferredCompilationCallback> >&&) 13 0x1022e1c9f JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue> const&, WTF::Ref<JSC::DeferredCompilationCallback, WTF::DumbPtrTraits<JSC::DeferredCompilationCallback> >&&) 14 0x1027dd5cc operationOptimize 15 0x5e89e90e72b 16 0x101b012c9 vmEntryToJavaScript 17 0x1027561de JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 18 0x10275576f JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) 19 0x102a4f385 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 20 0x101545cfb runWithOptions(GlobalObject*, CommandLine&, bool&) 21 0x10151d31a jscmain(int, char**)::$_6::operator()(JSC::VM&, GlobalObject*, bool&) const 22 0x101500954 int runJSC<jscmain(int, char**)::$_6>(CommandLine const&, bool, jscmain(int, char**)::$_6 const&) 23 0x1014ff0e7 jscmain(int, char**) 24 0x1014fef4e main 25 0x7fff62e41015 start 26 0xc https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1926/steps/jscore-test/logs/stdio

Caio Lima

Comment 8 2018-12-19 17:55:31 PST

I'm looking into that now. (In reply to Ryan Haddad from comment #7) > It looks like this change caused 35 JSC tests to fail an assertion: > > ASSERTION FAILED: node() > /Volumes/Data/slave/highsierra-debug/build/Source/JavaScriptCore/dfg/ > DFGEdge.h(82) : JSC::DFG::UseKind JSC::DFG::Edge::useKind() const > 1 0x101610ec9 WTFCrash > 2 0x101611ebb WTFCrashWithInfo(int, char const*, char const*, int) > 3 0x10175245d JSC::DFG::Edge::useKind() const > 4 0x10176ff95 JSC::DFG::Node::binaryUseKind() > 5 0x102507d39 JSC::DFG::StrengthReductionPhase::handleNode() > 6 0x10250656f JSC::DFG::StrengthReductionPhase::run() > 7 0x102506351 bool > JSC::DFG::runAndLog<JSC::DFG::StrengthReductionPhase>(JSC::DFG:: > StrengthReductionPhase&) > 8 0x1024fe72e bool > JSC::DFG::runPhase<JSC::DFG::StrengthReductionPhase>(JSC::DFG::Graph&) > 9 0x1024fe6f5 JSC::DFG::performStrengthReduction(JSC::DFG::Graph&) > 10 0x10249a9d7 JSC::DFG::Plan::compileInThreadImpl() > 11 0x102499112 JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*) > 12 0x1022e21fe JSC::DFG::compileImpl(JSC::VM&, JSC::CodeBlock*, > JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, > JSC::Operands<JSC::JSValue> const&, > WTF::Ref<JSC::DeferredCompilationCallback, > WTF::DumbPtrTraits<JSC::DeferredCompilationCallback> >&&) > 13 0x1022e1c9f JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, > JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, > JSC::Operands<JSC::JSValue> const&, > WTF::Ref<JSC::DeferredCompilationCallback, > WTF::DumbPtrTraits<JSC::DeferredCompilationCallback> >&&) > 14 0x1027dd5cc operationOptimize > 15 0x5e89e90e72b > 16 0x101b012c9 vmEntryToJavaScript > 17 0x1027561de JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) > 18 0x10275576f JSC::Interpreter::executeProgram(JSC::SourceCode const&, > JSC::ExecState*, JSC::JSObject*) > 19 0x102a4f385 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, > JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) > 20 0x101545cfb runWithOptions(GlobalObject*, CommandLine&, bool&) > 21 0x10151d31a jscmain(int, char**)::$_6::operator()(JSC::VM&, > GlobalObject*, bool&) const > 22 0x101500954 int runJSC<jscmain(int, char**)::$_6>(CommandLine const&, > bool, jscmain(int, char**)::$_6 const&) > 23 0x1014ff0e7 jscmain(int, char**) > 24 0x1014fef4e main > 25 0x7fff62e41015 start > 26 0xc > > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1926/steps/jscore- > test/logs/stdio

Caio Lima

Comment 9 2018-12-19 18:19:36 PST

Handling the issue in https://bugs.webkit.org/show_bug.cgi?id=192908

Keith Miller

Comment 10 2018-12-19 23:32:46 PST

I think we should revert this until we can reland it with a fix. It's not great to leave failing tests in the tree for an extended period of time.

WebKit Commit Bot

Comment 11 2018-12-19 23:33:42 PST

Re-opened since this is blocked by bug 192921

Caio Lima

Comment 12 2018-12-20 02:28:44 PST

*** Bug 192908 has been marked as a duplicate of this bug. ***

Caio Lima

Comment 13 2018-12-20 02:32:38 PST

(In reply to Keith Miller from comment #10) > I think we should revert this until we can reland it with a fix. It's not > great to leave failing tests in the tree for an extended period of time. Sorry for leaving this so long and thx for the rollback.

Caio Lima

Comment 14 2018-12-20 02:37:51 PST

Created attachment 357805 [details] Patch

Caio Lima

Comment 15 2018-12-20 02:40:01 PST

(In reply to Caio Lima from comment #14) > Created attachment 357805 [details] > Patch The difference from previous version is that we need to avoid execution of 'm_node->binaryUseKind()' after performing String Folding into DFGStrengthReductionPhase.cpp. I added a break into line 375

Yusuke Suzuki

Comment 16 2018-12-20 02:53:18 PST

Comment on attachment 357805 [details] Patch r=me

Caio Lima

Comment 17 2018-12-20 03:33:20 PST

Comment on attachment 357805 [details] Patch Thank you for the review!

WebKit Commit Bot

Comment 18 2018-12-20 03:59:12 PST

Comment on attachment 357805 [details] Patch Clearing flags on attachment: 357805 Committed r239438: <https://trac.webkit.org/changeset/239438>

WebKit Commit Bot

Comment 19 2018-12-20 03:59:14 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.