1480587 - add atomic operation support for aarch64 windows

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

This could be as simple as defining a case for _M_ARM64 to include AtomicOperations-feeling-lucky.h, but it would be splendid if we had real atomic op support.

Comment 1

[Lars T Hansen [:lth]]

Yeah, I keep meaning to write the code that jit-generates known-good known-appropriate atomics for all our jit-supporting architectures, but other projects keep getting in the way...

Comment 2

[Nicolas B. Pierron [:nbp]]

Tell me if I am wrong, but this feature should not block Sean form adding the Arm64 IonMonkey backend, as long as SharedArrayBuffer are behind a flag[1] ? Or do we have any other use of Atomics?

Also, should this a blocker for Bug 1480548, if the only use of Atomics is currently disabled?

[1] javascript.options.shared_memory

Comment 3

[Nathan Froyd [:froydnj]]

This is a blocker for actually compiling the JS engine on AArch64 Windows, since the functions for atomic operations are used unconditionally...hence the big #ifdef chain in the AtomicOperations.h header.  Even if the pref is off, we still need things to compile.

The good news is that tweaking the existing MSVC header slightly will work just fine--at least to compile things--on AArch64 Windows; I just need to submit the patch or have somebody else write the patch for me. :)

Comment 4

[Lars T Hansen [:lth]]

I'm not set up to write the patch but I'm happy to review, of course.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: add support for atomic operations on AArch64 Windows

The patch is simple enough.  Two things to note here:

- I don't know what to do about the naming of the header file.  I realize that
  it's a little weird to include an x86 file on AArch64...should I rename it?
  Where should it go?

- The substitution in fenceSeqCst requires some justification.  MemoryBarrier
  on x86oid processors is defined as __faststorefence, which is documented
  here: https://docs.microsoft.com/en-us/cpp/intrinsics/faststorefence  The
  documentation states that the effect is comparable to but faster than the
  _mm_mfence intrinsic.

  MemoryBarrier on ARM and AArch64 translates to the __dmb intrinsic, which
  seems correct.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 9002826 [details] [diff] [review]
add support for atomic operations on AArch64 Windows

Review of attachment 9002826 [details] [diff] [review]:
-----------------------------------------------------------------

For hack value: There's vigorous debate on the 'net about whether __faststorefence is faster than _mm_mfence.  __faststorefence generates a LOCK OR with memory, this is effectively equivalent to the LOCK ADD 0, [rsp] trick that Linux uses for a barrier on older systems.  In the past, a message I saw from the Java team at Sun/Oracle stated that whether one is better than the other is microarchitecture-dependent.  Microsoft's documentation states that __faststorefence is better on Intel and mfence is better on AMD64...  Don't we love it.

This particular code is not performance critical so substituting for MemoryBarrier if that is a platform-independent value is fine with me.

About the naming and placement of the header file.  Canonically we fork the file and place it in jit/arm64/AtomicOperations-arm64-msvc.h, and rename the one that's already in that directory to jit/arm64/AtomicOperations-arm64-gcc.h.  This duplicates code but has served us well in avoiding ifdeffery so far.  If you have the energy for it I think you should do that, as it keeps things simple.

I'm making good progress on jitting the atomic ops, and once that code is ready I foresee a substantial cleanup of some of these files anyway.  For Tier 1 platforms we'll only have support for jitting with a fallback to shared feeling-lucky implementations (one for gcc, one for msvc).  Thus a number of these apparently duplicated files will disappear.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/796f67446c43
add support for atomic operations on AArch64 Windows; r=lth

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/796f67446c43