Summary: | [ARM] Disable Inline Caching on ARMv7 traditional until proper fix | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||
Component: | JavaScriptCore | Assignee: | Csaba Osztrogonác <ossy> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, keith_miller, mark.lam, msaboff, ossy, saam | ||||||||
Priority: | P2 | ||||||||||
Version: | Other | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 159408 | ||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2016-07-14 05:15:12 PDT
Created attachment 283632 [details]
Patch
Comment on attachment 283632 [details] Patch Clearing flags on attachment: 283632 Committed r203272: <http://trac.webkit.org/changeset/203272> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 284851 [details] Patch Let's disable MathIC's too until proper fix. The ARM traditional port have been broken 6 weeks ago and more fixes are needed, for example bug160295, bug159720. Comment on attachment 284851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284851&action=review > Source/JavaScriptCore/jit/JITMathIC.h:71 > +#if CPU(ARM_TRADITIONAL) > + // FIXME: Remove this workaround once the proper fixes are landed. > + // [ARM] Disable Inline Caching on ARMv7 traditional until proper fix > + // https://bugs.webkit.org/show_bug.cgi?id=159759 > + return false; > +#endif Actually, I realized a better solution is to force the result to be JITMathICInlineResult::GenerateFullSnippet. You can accomplish that by having a compile-time conditional around m_generator.generateInline(...) below. This will revert to the behavior of the old code. This will be much faster than forcing a call on every add/mul/etc. (In reply to comment #6) > Comment on attachment 284851 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284851&action=review > > > Source/JavaScriptCore/jit/JITMathIC.h:71 > > +#if CPU(ARM_TRADITIONAL) > > + // FIXME: Remove this workaround once the proper fixes are landed. > > + // [ARM] Disable Inline Caching on ARMv7 traditional until proper fix > > + // https://bugs.webkit.org/show_bug.cgi?id=159759 > > + return false; > > +#endif > > Actually, I realized a better solution is to force the result to be > JITMathICInlineResult::GenerateFullSnippet. > You can accomplish that by having a compile-time conditional around > m_generator.generateInline(...) below. > This will revert to the behavior of the old code. This will be much faster > than forcing a call on every add/mul/etc. Did you mean a similar workaround? (of course with CPU(ARM_TRADITIONAL) guard) - JITMathICInlineResult result = m_generator.generateInline(jit, state); + JITMathICInlineResult result = JITMathICInlineResult::GenerateFullSnippet; I made a quick test, it decreased the number of crashes from 4500 to 3500, but it is so far from the optimal number zero. :( I'll check what causes the crashes on a debug build in 1-2 hours. I got the following crash backtraces with the patch you suggested: ASSERTION FAILED: linkBuffer.isValid() ../../Source/JavaScriptCore/jit/JITMathIC.h(130) : JSC::JITMathIC<Generator>::generateOutOfLine(JSC::VM&, JSC::CdeBlock*, JSC::FunctionPtr)::<lambda()> [with GeneratorType = JSC::JITAddGenerator] 1 0x41c53ad8 WTFCrash 2 0x41764034 JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPr)::{lambda()#1}::operator()() const 3 0x417643fc JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPr) 4 0x4175acb4 Illegal instruction ERROR: Unexpected exit code: 132 ---------------------- ASSERTION FAILED: static_cast<ptrdiff_t>(inlineSize) <= MacroAssembler::maxJumpReplacementSize() ../../Source/JavaScriptCore/jit/JITMathIC.h(81) : bool JSC::JITMathIC<Generator>::generateInline(JSC::CCallHelpers&, JSC::athICGenerationState&, bool) [with GeneratorType = JSC::JITAddGenerator] 1 0x41b65ad8 WTFCrash 2 0x41441788 JSC::JITMathIC<JSC::JITAddGenerator>::generateInline(JSC::CCallHelpers&, JSC::MathICGenerationState&, bool) 3 0x41643f50 void JSC::JIT::emitMathICFast<JSC::JITAddGenerator, long long (*)(JSC::ExecState*, long long, long long, JS::ArithProfile*), long long (*)(JSC::ExecState*, long long, long long)>(JSC::JITMathIC<JSC::JITAddGenerator>*, JSC::Instruction*, long long (*)(JSC::ExecStat*, long long, long long, JSC::ArithProfile*), long long (*)(JSC::ExecState*, long long, long long)) 4 0x41641148 JSC::JIT::emit_op_add(JSC::Instruction*) 5 0x4162f710 JSC::JIT::privateCompileMainPass() 6 0x41632868 JSC::JIT::compileWithoutLinking(JSC::JITCompilationEffort) 7 0x41633e34 JSC::JIT::privateCompile(JSC::JITCompilationEffort) 8 0x41691630 JSC::JIT::compile(JSC::VM*, JSC::CodeBlock*, JSC::JITCompilationEffort) 9 0x41861300 10 0x418614d8 JSC::ScriptExecutable::prepareForExecutionImpl(JSC::ExecState*, JSC::JSFunction*, JSC::JSScope*, JSC::CodeSecializationKind) 11 0x415fb35c JSC::ScriptExecutable::prepareForExecution(JSC::ExecState*, JSC::JSFunction*, JSC::JSScope*, JSC::CodeSpecilizationKind) 12 0x4166636c Illegal instruction ERROR: Unexpected exit code: 132 Do you have a better idea how to workaround this regression? Or should we push the previous change I suggested? Comment on attachment 284851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284851&action=review >>> Source/JavaScriptCore/jit/JITMathIC.h:71 >>> +#endif >> >> Actually, I realized a better solution is to force the result to be JITMathICInlineResult::GenerateFullSnippet. >> You can accomplish that by having a compile-time conditional around m_generator.generateInline(...) below. >> This will revert to the behavior of the old code. This will be much faster than forcing a call on every add/mul/etc. > > Did you mean a similar workaround? (of course with CPU(ARM_TRADITIONAL) guard) > - JITMathICInlineResult result = m_generator.generateInline(jit, state); > + JITMathICInlineResult result = JITMathICInlineResult::GenerateFullSnippet; > > I made a quick test, it decreased the number of crashes from > 4500 to 3500, but it is so far from the optimal number zero. :( > I'll check what causes the crashes on a debug build in 1-2 hours. Sorry, so you want to both do that and then also put the if (ArithProfile* ...) branch behind a compile flag so it doesn't execute in arm traditional Created attachment 285097 [details]
Patch
(In reply to comment #10) > Created attachment 285097 [details] > Patch Did you mean this workaround? Unfortunately it doesn't help too lot. There are still 1066 crashes with this patch, it means that all mozilla-tests.yaml/****.js.mozilla-baseline tests crashes because of "Illegal instruction". (In reply to comment #11) > (In reply to comment #10) > > Created attachment 285097 [details] > > Patch > > Did you mean this workaround? Unfortunately it doesn't help too lot. > There are still 1066 crashes with this patch, it means that all > mozilla-tests.yaml/****.js.mozilla-baseline tests crashes because > of "Illegal instruction". I still get the following assertions with this workaround: ASSERTION FAILED: linkBuffer.isValid() ../../Source/JavaScriptCore/jit/JITMathIC.h(137) : JSC::JITMathIC<Generator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPtr)::<lambda()> [with GeneratorType = JSC::JITAddGenerator] 1 0xb638eb44 WTFCrash 2 0xb5e9e0e4 JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPtr)::{lambda()#1}::operator()() const 3 0xb5e9e778 JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPtr) 4 0xb5e94ab4 Segmentation fault ERROR: Unexpected exit code: 139 Comment on attachment 284851 [details]
Patch
The ARM assembler is broken since 6 weeks and I will take my
holiday since now until the end of August and don't want to
left ARM assembler broken for this long period.
Please reconsider your previous r- on this workaround.
Comment on attachment 285097 [details]
Patch
clearing r? , because it doesn't work
Comment on attachment 284851 [details] Patch Clearing flags on attachment: 284851 Committed r204025: <http://trac.webkit.org/changeset/204025> All reviewed patches have been landed. Closing bug. |