1630359 - On x86 and x64, there is code that assumes JitSupportsSimd() is always true

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Lars T Hansen [:lth]]

There are several vaguely related SIMD guards in the current code (pre-wasm SIMD):

• js::jit::SupportsSimd, a bool constexpr defined in each machine assembler. This flag is meant to be used in static_asserts throughout the code that will fail if we enable it and thus will point to code that needs to be updated for SIMD.

• jit::JitSupportsSimd(), which returns the value of MacroAssembler::SupportsSimd().

• MacroAssembler::SupportsSimd(), a predicate defined in each machine assembler. This predicate returns either (a) the value of the SupportsSimd flag (on ARM, ARM64, MIPS32, and MIPS64), or (b) the value of CPUInfo::IsSSE2Present() (on X86 and X64). Since nearly 100% of our customers' x86 systems have SSE2 [1][2], this flag is effectively always true.

Various parts of the Jit code guard their logic on jit::JitSupportsSimd(), for example, when generating code for saving and restoring register sets.

If I make JitSupportsSimd() return false, various [3] wasm and JS jit-tests start failing (SEGV, reasons not yet investigated) on x64 and x86. Thus there's platform code somewhere that assumes that this predicate is never false and fails to test it. As the predicate will likely turn into an #ifdef when I land wasm SIMD, the problem has to be rectified somehow.

[1] https://firefoxgraphics.github.io/telemetry/#view=system
[2] I thought SSE2 was a hard requirement but I haven't found a guard that enforces that
[3] wasm/atomicity.js is the one I found first but there are many, many others, and some of them are failed assertions in GC and OSI code

Comment 1

[Lars T Hansen [:lth]]

Looks like an incorrect (unguarded) use of PushRegsInMask(AllRegs) in generateInvalidator, an easy fix.

Comment 2

[Lars T Hansen [:lth]]

Attachment: Bug 1630359 - Create correct register dumps when SIMD disabled. r?bbouvier

On x86 and x64, the register dump has space for the full XMM FP
registers, but PushRegistersInMask will reduce the register set down
to quadword if JitSupportsSimd() is false. Thus register dump areas
must be created by another mechanism than PushRegistersInMask when
!JitSupportsSimd(). The platform code did this in the case of
bailouts but got it wrong for invalidations; this patch fixes that for
both platforms.

We never found this bug because JitSupportsSimd() currently always
returns true on x86 and x64, as it only checks HasSSE2(), and that is
always true.

Additionally, make JitSupportsSimd() guard its return value on the
js::jit::SupportsSimd flag, which it probably should have been doing
all along (it does this on other platforms). Since that flag is
always false, JitSupportsSimd() will now always return false, there
being no SIMD on x86 or x64 yet.

Discuss: DumpAllRegs could be moved to x86-shared code, eg, we could
create a Trampoline-x86-shared.cpp file. Not sure if it's worth it.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f3d920fea5b
Create correct register dumps when SIMD disabled. r=bbouvier

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7f3d920fea5b