Bug 192723

Summary: [BigInt] We should enable CSE into arithmetic operations that speculate BigIntUse
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: 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: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 192921    
Bug Blocks:    
Attachments:
Description Flags
Proposed Patch
none
Patch none

Description Caio Lima 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.
Comment 1 Caio Lima 2018-12-17 05:51:11 PST
Created attachment 357436 [details]
Proposed Patch
Comment 2 Saam Barati 2018-12-18 23:33:14 PST
Comment on attachment 357436 [details]
Proposed Patch

r=me
Comment 3 Caio Lima 2018-12-19 03:47:00 PST
Comment on attachment 357436 [details]
Proposed Patch

Thank you very much for the review.
Comment 4 WebKit Commit Bot 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>
Comment 5 WebKit Commit Bot 2018-12-19 04:12:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2018-12-19 04:13:38 PST
<rdar://problem/46838118>
Comment 7 Ryan Haddad 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
Comment 8 Caio Lima 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
Comment 9 Caio Lima 2018-12-19 18:19:36 PST
Handling the issue in https://bugs.webkit.org/show_bug.cgi?id=192908
Comment 10 Keith Miller 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.
Comment 11 WebKit Commit Bot 2018-12-19 23:33:42 PST
Re-opened since this is blocked by bug 192921
Comment 12 Caio Lima 2018-12-20 02:28:44 PST
*** Bug 192908 has been marked as a duplicate of this bug. ***
Comment 13 Caio Lima 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.
Comment 14 Caio Lima 2018-12-20 02:37:51 PST
Created attachment 357805 [details]
Patch
Comment 15 Caio Lima 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
Comment 16 Yusuke Suzuki 2018-12-20 02:53:18 PST
Comment on attachment 357805 [details]
Patch

r=me
Comment 17 Caio Lima 2018-12-20 03:33:20 PST
Comment on attachment 357805 [details]
Patch

Thank you for the review!
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2018-12-20 03:59:14 PST
All reviewed patches have been landed.  Closing bug.