1199535 - IonMonkey: MIPS: Implement AtomicOperations shared

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Implement AtomicOperations-mips-shared for mips32 and mips64.

Comment 1

[hev [:hev]]

Attachment: IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch

Comment 2

[hev [:hev]]

Comment on attachment 8653837 [details] [diff] [review]
IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch

Note: depends bug 1194139 part 3

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8653837 [details] [diff] [review]
IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch

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

I have no idea what this file is meant for, I will forward the review to Lars.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8653837 [details] [diff] [review]
IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch

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

r- for the questionable implementation of isLockFree8, see comments - let's discuss.  Otherwise looks OK.

::: js/src/jit/mips-shared/AtomicOperations-mips-shared.h
@@ +40,1 @@
>  }

I doubt this definition is correct, but I don't know enough about MIPS or the compilers used on MIPS to say for sure.

To start with what I know, MIPS32 only has 32-bit LL/SC instructions, so it's surprising that isLockFree8() would be true on MIPS32.

Furthermore, I'd be astonished if MIPS64 does not have 64-bit LL/SC instructions, so it would be surprising if isLockFree8() would be false on MIPS64 even for the SYNC intinsics.  (Maybe our MIPS64 code hasn't landed yet but this is mips-shared code, after all.)

In either case, what matters here is that these methods do what the JIT will do on the platform in question.  (Ie these methods are C++ manifestations of JIT functionality.)  And surely on MIPS64 the JIT would use 64-bit LL/SC for the atomic operations.

Comment 5

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #4)
> Comment on attachment 8653837 [details] [diff] [review]
> IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch
> 
> Review of attachment 8653837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the questionable implementation of isLockFree8, see comments - let's
> discuss.  Otherwise looks OK.
> 
> ::: js/src/jit/mips-shared/AtomicOperations-mips-shared.h
> @@ +40,1 @@
> >  }
> 
> I doubt this definition is correct, but I don't know enough about MIPS or
> the compilers used on MIPS to say for sure.
> 
> To start with what I know, MIPS32 only has 32-bit LL/SC instructions, so
> it's surprising that isLockFree8() would be true on MIPS32.

Thank you. MIPS32 only has 32-bit LL/SC instructions. MIPS64 have 32-bit LL/SC and 64-bit DLL/DSC instructions. still, 8/16/32-bit atomics can implemented by LL/SC instructions on MIPS32, and 64-bit atomics by DLL/DSC on MIPS64.

For example, 8-bit atomics add on MIPS32:
atomic_add8(address, byte):
1. r0 = address & ~3; // make address aligned on 4-byte.
2. r1 = address - r0; // calculate target 8-bit offset based on aligned address.
3. r2 = 0xff << r1; // make target 8-bit mask.
4. r3 = ll(r0); // linked-load 4-byte from aligned addr.
5. r4 = r3 + (byte << r1); // add
6. r4 = (r3 & ~r2) | (r4 & r2); // extract target 8-bit from r4 and insert into r3.
7. if 0 == sc(r0, r4) then goto step 4. // if fail, retry.

> 
> Furthermore, I'd be astonished if MIPS64 does not have 64-bit LL/SC
> instructions, so it would be surprising if isLockFree8() would be false on
> MIPS64 even for the SYNC intinsics.  (Maybe our MIPS64 code hasn't landed
> yet but this is mips-shared code, after all.)
> 
> In either case, what matters here is that these methods do what the JIT will
> do on the platform in question.  (Ie these methods are C++ manifestations of
> JIT functionality.)  And surely on MIPS64 the JIT would use 64-bit LL/SC for
> the atomic operations.

This code implemented for MIPS64 first, currently jit for mips32 have many issues,
so I can't do a deep tests for it. I think __atomic_always_lock_free with 64-bit 
should return false on MIPS32 (O32 ABI). I will update this patch soon after. Thanks!

Comment 6

[hev [:hev]]

Attachment: IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch

Comment 7

[Lars T Hansen [:lth]]

(In reply to Heiher [:hev] from comment #5)
> (In reply to Lars T Hansen [:lth] from comment #4)
> > Comment on attachment 8653837 [details] [diff] [review]
> > IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch
> > 
> > Review of attachment 8653837 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r- for the questionable implementation of isLockFree8, see comments - let's
> > discuss.  Otherwise looks OK.
> > 
> > ::: js/src/jit/mips-shared/AtomicOperations-mips-shared.h
> > @@ +40,1 @@
> > >  }
> > 
> > I doubt this definition is correct, but I don't know enough about MIPS or
> > the compilers used on MIPS to say for sure.
> > 
> > To start with what I know, MIPS32 only has 32-bit LL/SC instructions, so
> > it's surprising that isLockFree8() would be true on MIPS32.
> 
> Thank you. MIPS32 only has 32-bit LL/SC instructions. MIPS64 have 32-bit
> LL/SC and 64-bit DLL/DSC instructions. still, 8/16/32-bit atomics can
> implemented by LL/SC instructions on MIPS32, and 64-bit atomics by DLL/DSC
> on MIPS64.

That's what I've been figuring too, which is why we only have an isLockFree8() and not a general isLockFree() primitive (code higher up in the stack just assumes that 1/2/4 bytes are lock free).

I'm not sure how smart that was of me, but these are internals so we can adjust it as we go along.

It's actually kind of funny that right now we have no 64-bit atomics exposed to JS code on any platform, so isLockFree8() is technically redundant.  It was invented to support float64 atomics, which few people wanted.  We'll want it for int64 atomics, though.

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8657016 [details] [diff] [review]
IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch

What does _MIPS_SIM mean?  Does it have something to do with the simulator?

(I think that question has made me realize that the code in AtomicOperations may have to be different on a simulator than on a native CPU, and/or the simulator must defer to AtomicOperations for its implementation of atomics, though that's kind of weird.  Investigating.)

Comment 9

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #8)
> Comment on attachment 8657016 [details] [diff] [review]
> IonMonkey-MIPS-Implement-AtomicOperations-mips-shared.patch
> 
> What does _MIPS_SIM mean?  Does it have something to do with the simulator?

This macro is pre-defined by compiler, if on o32 abi, then _MIPS_SIM == _ABIO32, else if on n64 abi, then _MIPS_SIM == _ABI64. It has nothing to do with the simulator.

> 
> (I think that question has made me realize that the code in AtomicOperations
> may have to be different on a simulator than on a native CPU, and/or the
> simulator must defer to AtomicOperations for its implementation of atomics,
> though that's kind of weird.  Investigating.)

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18c50ebe2af3

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18c50ebe2af3