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

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

[Caio Lima]

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.

[Caio Lima]

Created attachment 357436 [details]
Proposed Patch

[Saam Barati]

Comment on attachment 357436 [details]
Proposed Patch

r=me

[Caio Lima]

Comment on attachment 357436 [details]
Proposed Patch

Thank you very much for the review.

[WebKit Commit Bot]

Comment on attachment 357436 [details]
Proposed Patch

Clearing flags on attachment: 357436

Committed r239377: <https://trac.webkit.org/changeset/239377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46838118>

[Ryan Haddad]

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]

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]

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

[Keith Miller]

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]

Re-opened since this is blocked by bug 192921

[Caio Lima]

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

[Caio Lima]

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

Created attachment 357805 [details]
Patch

[Caio Lima]

(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 on attachment 357805 [details]
Patch

r=me

[Caio Lima]

Comment on attachment 357805 [details]
Patch

Thank you for the review!

[WebKit Commit Bot]

Comment on attachment 357805 [details]
Patch

Clearing flags on attachment: 357805

Committed r239438: <https://trac.webkit.org/changeset/239438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.