Bug 159759 - [ARM] Disable Inline Caching on ARMv7 traditional until proper fix
Summary: [ARM] Disable Inline Caching on ARMv7 traditional until proper fix
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 159408
  Show dependency treegraph
 
Reported: 2016-07-14 05:15 PDT by Csaba Osztrogonác
Modified: 2016-08-02 08:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.46 KB, patch)
2016-07-14 05:32 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (1.46 KB, patch)
2016-07-29 00:55 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2016-08-02 04:02 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2016-07-14 05:15:12 PDT
https://trac.webkit.org/changeset/202214 broke inline caching
on ARMv7 traditional (ARM instruction set / ARMAssembler)

bug159408 is to fix this regression step by step.
But ARM traditional assembler is completely broken
since a month ago, so I propose disabling inline
caching on it until all fixes are reviewed and landed.
Comment 1 Csaba Osztrogonác 2016-07-14 05:32:38 PDT
Created attachment 283632 [details]
Patch
Comment 2 WebKit Commit Bot 2016-07-15 02:44:22 PDT
Comment on attachment 283632 [details]
Patch

Clearing flags on attachment: 283632

Committed r203272: <http://trac.webkit.org/changeset/203272>
Comment 3 WebKit Commit Bot 2016-07-15 02:44:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Csaba Osztrogonác 2016-07-29 00:55:38 PDT
Reopening to attach new patch.
Comment 5 Csaba Osztrogonác 2016-07-29 00:55:42 PDT
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 6 Saam Barati 2016-07-31 14:03:33 PDT
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.
Comment 7 Csaba Osztrogonác 2016-08-01 03:15:59 PDT
(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.
Comment 8 Csaba Osztrogonác 2016-08-01 04:29:34 PDT
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 9 Saam Barati 2016-08-01 08:56:39 PDT
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
Comment 10 Csaba Osztrogonác 2016-08-02 04:02:31 PDT
Created attachment 285097 [details]
Patch
Comment 11 Csaba Osztrogonác 2016-08-02 04:57:05 PDT
(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".
Comment 12 Csaba Osztrogonác 2016-08-02 05:22:20 PDT
(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 13 Csaba Osztrogonác 2016-08-02 05:30:09 PDT
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 14 Csaba Osztrogonác 2016-08-02 05:30:56 PDT
Comment on attachment 285097 [details]
Patch

clearing r? , because it doesn't work
Comment 15 WebKit Commit Bot 2016-08-02 08:38:42 PDT
Comment on attachment 284851 [details]
Patch

Clearing flags on attachment: 284851

Committed r204025: <http://trac.webkit.org/changeset/204025>
Comment 16 WebKit Commit Bot 2016-08-02 08:38:47 PDT
All reviewed patches have been landed.  Closing bug.