1724375 - Tidy up SSE flags override code

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

The shell allows the SSE settings to be overridden for testing. But as the engine may jit some code very eagerly on startup (for atomics) those flags may become set to hardware defaults before the shell has a chance to set them, and so they have to be reset before the shell can set them. The reset code is brittle; if multiple flags are provided to the shell, it's not completely obvious what will happen with the flags. This can be made un-brittle.

Comment 1

[Lars T Hansen [:lth]]

Attachment: Bug 1724375 - Clean up SSE flags override code. r?jandem

The SSE flags override must be sure to reset the SSE flags first, for
reasons already stated in the code. But if multiple flags are present
the semantics of the flag overrides is a little opaque. We clean this
up.

There is a minor semantic change here in that the most restrictive
setting is now kept, while previously the last setting was kept, and
this was (given the structure of the calling code) always the least
restrictive setting. In practical terms this should not matter, and
if it does we can fix fallout later.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6a21c4f4bbe
Clean up SSE flags override code.  r=jandem

Comment 3

[Cristian Brindusan [:cbrindusan]]

Backed out changeset a6a21c4f4bbe (Bug 1724375) for causing SM bustages in Assembler-x86-shared.h.
https://hg.mozilla.org/integration/autoland/rev/446a83f4be0869561a95233f95e85e99bcea63ea

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=a6a21c4f4bbe533350adec82523907148425aa50&selectedTaskRun=LfX8WYaTQdWopnwoEPWzpQ.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=347668807&repo=autoland&lineNumber=2434

Comment 4

[Lars T Hansen [:lth]]

That's annoying. If there's a race condition there now there was a race condition there always, except now it's beeing exposed more easily by the eager resetting of the flags. And it's doubly annoying since this is shell-only testing code.

Comment 5

[Jan de Mooij [:jandem]]

Do you know what's causing this? Without checking the code, I thought we were only setting this based on the shell flags in main() and then not touching it.

Comment 6

[Lars T Hansen [:lth]]

It looks like what happens is that the flags are reset, then the script spawns a bunch of parallel asm.js compiles. These will not really be synchronized. Then one of the threads will look at the flags, discover that they're not set, and compute them, and write them. Then another thread comes in and reads them. Since they're not atomic, it's a synchronization violation.

Previously this would not be a problem because eager compilation of the atomics would set the flags and the shell flags would not be present. Now that I always reset, it becomes a problem.

A quickfix is to not reset the flags eagerly, but only once, when the first switch is seen. Technically we still have a bug but nobody will notice, until such a time as we stop compiling the atomics eagerly :-}

Better might be for the shell (or indeed the engine as a whole) to force-compute the flags always after processing switches etc, this seems cleaner anyway. Not sure how much time I want to invest in this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74c5638ad6d7
Clean up SSE flags override code.  r=jandem

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/74c5638ad6d7