1203044 - IonMonkey: MIPS32: Atomics operations should throw on oob access

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Some atomics operations not implemented on mips32, I just attach a part of this patch. I will implement atomics that marked as 'NYI' for mips32 in Bug 1090957 after Bug 1194139 finished.

Comment 1

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Atomics-operations-should-throw-on-.patch

thanks!

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8658589 [details] [diff] [review]
0001-IonMonkey-MIPS32-Atomics-operations-should-throw-on-.patch

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

I guess this is OK.  The code seems really branchy to me but I will assume you know best, I see the existing load code uses a similar pattern.

I would have expected to see changes to the implementation of compareExchange, add, etc, but perhaps those are implemented with VM callouts?

What worries me with load and store is that there are no memory barriers here.  That would have been correct on MIPS in the old days when it had a sequentially consistent memory model, but my understanding now is that it has a weak memory model a la ARM.  If so, you need barriers before and after the loads and stores, cf ARM.  Can you tell me why those are missing here?

Comment 3

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #2)
> Comment on attachment 8658589 [details] [diff] [review]
> 0001-IonMonkey-MIPS32-Atomics-operations-should-throw-on-.patch
> 
> Review of attachment 8658589 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this is OK.  The code seems really branchy to me but I will assume
> you know best, I see the existing load code uses a similar pattern.
> 
> I would have expected to see changes to the implementation of
> compareExchange, add, etc, but perhaps those are implemented with VM
> callouts?
> 
> What worries me with load and store is that there are no memory barriers
> here.  That would have been correct on MIPS in the old days when it had a
> sequentially consistent memory model, but my understanding now is that it
> has a weak memory model a la ARM.  If so, you need barriers before and after
> the loads and stores, cf ARM.  Can you tell me why those are missing here?

Thank you. I plan implement these atomics (contain memoryBarrier) in Bug 1090957. I'm doing extract shareable code from MacroAssemblerMIPS to mips-shared directory, so i think attch atomics after that finished might a good way. If possible, please help review this patch in mips64 is ok? thx.

https://github.com/heiher/gecko-dev/commit/a0c6a43fea16bd49af011ecbce253d4afabd6093

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8658589 [details] [diff] [review]
0001-IonMonkey-MIPS32-Atomics-operations-should-throw-on-.patch

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

Fair enough.

Comment 5

[Lars T Hansen [:lth]]

BTW, I only scanned it briefly - not a proper review at this point - but the MIPS64 code you posted looks right.  Thanks.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec890000d297

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec890000d297