Bug 196884 – r244079 logically broke shouldSpeculateInt52

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

[Saam Barati]

We have code which is like:
if (shouldSpeculateInt32()) ... int32 stuff
else if (shouldSpeculateInt52()) ... int52 stuff

However, we always test for shouldSpeculateInt32 prior to shouldSpeculateInt52. But shouldSpeculateInt52 should allow int32 to flow into it. Think about a program like this:

function foo(b, y) {
    let x;
    if (b)
        x = 0; // int32
    else
        x = 0xffffffffff; // int52
    return y + x;
}

We should totally pick int52 for x, even though we weren't.

[Saam Barati]

Created attachment 367362 [details]
patch

[Yusuke Suzuki]

Comment on attachment 367362 [details]
patch

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

r=me

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:738
> +            if (type == SpecAnyIntAsDouble && enableInt52() && int52AwareSpeculationFromValue(m_currentNode->asJSValue()) == SpecNonInt32AsInt52)
> +                type = SpecNonInt32AsInt52;

Why don't we say SpecInt32AsInt52 here for Int32 values in double?

[Saam Barati]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 367362 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367362&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:738
> > +            if (type == SpecAnyIntAsDouble && enableInt52() && int52AwareSpeculationFromValue(m_currentNode->asJSValue()) == SpecNonInt32AsInt52)
> > +                type = SpecNonInt32AsInt52;
> 
> Why don't we say SpecInt32AsInt52 here for Int32 values in double?

I spoke with Yusuke on IRC. We're going to pick what type int52AwareSpeculationFromValue gives us

[Saam Barati]

Created attachment 367364 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 367364 [details]
patch for landing

Clearing flags on attachment: 367364

Committed r244238: <https://trac.webkit.org/changeset/244238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49871877>

[Ryan Haddad]

This change caused ~20 tests to fail an assertion the JSC Debug bot:

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20(Tests)/builds/2587/steps/jscore-test/logs/stdio

stress/arith-abs-with-bitwise-or-zero.js.no-ftl: ASSERTION FAILED: isAnyInt52Speculation(m_type)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: ./dfg/DFGAbstractValue.cpp(443) : void JSC::DFG::AbstractValue::checkConsistency() const
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 1   0x1092f8019 WTFCrash
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 2   0x1092faffb WTFCrashWithInfo(int, char const*, char const*, int)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 3   0x109e57ee9 JSC::DFG::AbstractValue::checkConsistency() const
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 4   0x109e591d0 JSC::DFG::AbstractValue::mergeOSREntryValue(JSC::DFG::Graph&, JSC::JSValue)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 5   0x109efa5fe JSC::DFG::CFAPhase::injectOSR(JSC::DFG::BasicBlock*)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 6   0x109efa7b2 JSC::DFG::CFAPhase::performBlockCFA(JSC::DFG::BasicBlock*)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 7   0x109efa3c7 JSC::DFG::CFAPhase::performForwardCFA()
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 8   0x109efa0b7 JSC::DFG::CFAPhase::run()
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 9   0x109ef9cb1 bool JSC::DFG::runAndLog<JSC::DFG::CFAPhase>(JSC::DFG::CFAPhase&)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 10  0x109ecc7ae bool JSC::DFG::runPhase<JSC::DFG::CFAPhase>(JSC::DFG::Graph&)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 11  0x109ecc775 JSC::DFG::performCFA(JSC::DFG::Graph&)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 12  0x10a1b05a0 JSC::DFG::Plan::compileInThreadImpl()
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 13  0x10a1aeb2f JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 14  0x10a256e96 JSC::DFG::Worklist::ThreadBody::work()
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 15  0x10930ef6f WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 16  0x10930eb59 WTF::Function<void ()>::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>::call()
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 17  0x10932249d WTF::Function<void ()>::operator()() const
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 18  0x1093b90f3 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 19  0x1093c17f5 WTF::wtfThreadEntryPoint(void*)
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 20  0x7fff58297661 _pthread_body
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 21  0x7fff5829750d _pthread_body
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 22  0x7fff58296bf9 thread_start
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: test_script_1749: line 2: 50271 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --validateExceptionChecks\=true --useDollarVM\=true --maxPerThreadStackUsage\=1572864 arith-abs-with-bitwise-or-zero.js )
stress/arith-abs-with-bitwise-or-zero.js.no-ftl: ERROR: Unexpected exit code: 139

[Saam Barati]

(In reply to Ryan Haddad from comment #8)
> This change caused ~20 tests to fail an assertion the JSC Debug bot:
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20JSC%20(Tests)/builds/2587/steps/jscore-test/
> logs/stdio
> 
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: ASSERTION FAILED:
> isAnyInt52Speculation(m_type)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl:
> ./dfg/DFGAbstractValue.cpp(443) : void
> JSC::DFG::AbstractValue::checkConsistency() const
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 1   0x1092f8019 WTFCrash
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 2   0x1092faffb
> WTFCrashWithInfo(int, char const*, char const*, int)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 3   0x109e57ee9
> JSC::DFG::AbstractValue::checkConsistency() const
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 4   0x109e591d0
> JSC::DFG::AbstractValue::mergeOSREntryValue(JSC::DFG::Graph&, JSC::JSValue)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 5   0x109efa5fe
> JSC::DFG::CFAPhase::injectOSR(JSC::DFG::BasicBlock*)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 6   0x109efa7b2
> JSC::DFG::CFAPhase::performBlockCFA(JSC::DFG::BasicBlock*)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 7   0x109efa3c7
> JSC::DFG::CFAPhase::performForwardCFA()
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 8   0x109efa0b7
> JSC::DFG::CFAPhase::run()
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 9   0x109ef9cb1 bool
> JSC::DFG::runAndLog<JSC::DFG::CFAPhase>(JSC::DFG::CFAPhase&)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 10  0x109ecc7ae bool
> JSC::DFG::runPhase<JSC::DFG::CFAPhase>(JSC::DFG::Graph&)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 11  0x109ecc775
> JSC::DFG::performCFA(JSC::DFG::Graph&)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 12  0x10a1b05a0
> JSC::DFG::Plan::compileInThreadImpl()
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 13  0x10a1aeb2f
> JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 14  0x10a256e96
> JSC::DFG::Worklist::ThreadBody::work()
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 15  0x10930ef6f
> WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()()
> const
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 16  0x10930eb59
> WTF::Function<void
> ()>::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker
> const&)::$_0>::call()
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 17  0x10932249d
> WTF::Function<void ()>::operator()() const
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 18  0x1093b90f3
> WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 19  0x1093c17f5
> WTF::wtfThreadEntryPoint(void*)
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 20  0x7fff58297661
> _pthread_body
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 21  0x7fff5829750d
> _pthread_body
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: 22  0x7fff58296bf9
> thread_start
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: test_script_1749: line 2:
> 50271 Segmentation fault: 11  ( "$@"
> ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false
> --useFunctionDotArguments\=true --validateExceptionChecks\=true
> --useDollarVM\=true --maxPerThreadStackUsage\=1572864
> arith-abs-with-bitwise-or-zero.js )
> stress/arith-abs-with-bitwise-or-zero.js.no-ftl: ERROR: Unexpected exit
> code: 139

Will look once I’m at a computer

[Saam Barati]

Fixin in:
https://bugs.webkit.org/show_bug.cgi?id=196918