Bug 138315

Summary: [JSC] Erratum (835769) in Cortex-A53
Product: WebKit Reporter: Akos Kiss <akiss>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, dbates, fpizlo, gyuyoung.kim, loki, msaboff, rakuco, ryuan.choi, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=197192
Description Flags
Proposed patch.
fpizlo: review+, fpizlo: commit-queue-
Updated patch none

Description Akos Kiss 2014-11-03 10:22:22 PST
There is an erratum (835769) in some Cortex-A53, which can only be worked around at code generation: It is possible for an A64 64-bit multiply-accumulate instruction to generate an incorrect result when immediately preceded by a memory instruction. In WebKit, that means 4 affected stages, potentially:

1. compilation with gcc or clang
2. processing LLInt asm sources with the offline assembler
3. MacroAssembler
4. LLVM in FTL

1. Fixes for both gcc and clang have already been submitted:
So, if building WebKit for Cortex-A53, the build system should detect whether the compiler tool chain has support for the workaround and apply it if available.

2 & 3. Offlineasm & MacroAssembler should be amended with the workaround.

4. LLVM trunk already has the workaround, so as soon as ARM64 (for Linux) has FTL working, WebKit can rely on LLVM generating correct code.
Comment 1 Filip Pizlo 2014-11-03 10:26:54 PST
Please make sure that whatever this fix is, it is possible to disable it.  It should be disabled by default on Darwin.
Comment 2 Akos Kiss 2014-11-03 10:31:58 PST
Created attachment 240860 [details]
Proposed patch.

Built with Tools/Scripts/build-jsc --gtk --cmakeargs="-DWTF_CPU_ARM64_CORTEXA53=ON", no regression.

Some extra comments:

1) The ARM64 assembler runs into the nop generating code path in 162 jsc tests. E.g., in sunspider-1.0/string-fasta.js.default it generates something like this below:

      57:< 1:-4>        GetLocal(@1, JS, Int32, arg1(B<Int32>/FlushedInt32), machine:arg1, R:Variables(7), bc#98) predicting Int32
            0x7fa947f30c:    ldur   w0, [fp, #56]
      58:< 2:-4>        ArithMul(Int32:@45, Int32:@57, Number, Int32, CheckOverflowAndNegativeZero, bc#98)
            0x7fa947f310:    nop   
            0x7fa947f314:    smaddl x1, w2, w0, x31

2) For now, offlineasm generates no extra nops, since the LLInt asm sources contain no patterns which would trigger the nop-inserting code. The offlineasm part of the patch has been tested with crafted code (of no real use), however, and the generated assembly was OK. E.g.:

    "\tldr x2, [x13, #16]\n"                                 // /home/akiss/devel/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1375
    "\tnop\n"                                                // /home/akiss/devel/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1376
    "\tmadd w1, w1, w1, wzr\n"                               // /home/akiss/devel/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1376
Comment 3 Filip Pizlo 2014-11-03 10:46:30 PST
Comment on attachment 240860 [details]
Proposed patch.

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

LGTM with the one suggestion.

> Source/JavaScriptCore/assembler/ARM64Assembler.h:1571
> +#if CPU(ARM64_CORTEXA53)

You could remove this #if and instead have an #if inside the nopCortexA53Fix835769() method. That would beautify the code quite a bit without any change in perf or behavior.
Comment 4 Akos Kiss 2014-11-03 12:10:51 PST
Created attachment 240866 [details]
Updated patch

Thanks for the suggestion. Applied.
Comment 5 WebKit Commit Bot 2014-11-03 23:36:15 PST
Comment on attachment 240866 [details]
Updated patch

Clearing flags on attachment: 240866

Committed r175514: <http://trac.webkit.org/changeset/175514>
Comment 6 WebKit Commit Bot 2014-11-03 23:36:19 PST
All reviewed patches have been landed.  Closing bug.