968524 - IonMonkey: Add atomic_inc, atomic_dec, and atomic_cmpxchg.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: patch

Requested by Shu for x86/x86_64. Tested in GDB.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8371113 [details] [diff] [review]
patch

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

Thanks! That looks good but IMO could take advantage of a slight refactoring. I am not closed on the topic but would like to hear your opinion first if you want to keep it that way.

::: js/src/assembler/assembler/X86Assembler.h
@@ +1183,5 @@
> +    }
> +    void lock_incl_m32(int offset, RegisterID base)
> +    {
> +        spew("lock incl  %s0x%x(%s)", PRETTY_PRINT_OFFSET(offset), nameIReg(base));
> +        m_formatter.oneByteOp(PRE_LOCK);

I see that it's already been the case to put directly the PRE_LOCK byte in the instruction function (for xaddl_rm), but it feels kinda redundant.  How about a lock() function in this file, that would be directly called in the Assembler-x86-shared functions? The only difference would be that the spewing will show the lock in a separate line, which is good too, as it's closer to what a disassembler would show.  This way, we don't need any lock_* functions anymore.

There are also 2 other existing uses (xaddl_rm x 2) which could take advantage of that, but that's up to you.

@@ +1221,5 @@
> +        m_formatter.oneByteOp(PRE_LOCK);
> +        m_formatter.oneByteOp(OP_GROUP5_Ev, GROUP5_OP_DEC, addr);
> +    }
> +
> +    void lock_cmpxchg(RegisterID src, int offset, RegisterID base)

Could you indicate in the name that it works only for 32 bits, please?

@@ +1225,5 @@
> +    void lock_cmpxchg(RegisterID src, int offset, RegisterID base)
> +    {
> +        // Note that 32-bit CMPXCHG performs comparison against %eax.
> +        // If %eax == [%base+offset], then %src -> [%base+offset].
> +        // Otherwise, [%base+offset] -> %eax.

Also, in the first case, ZF is set, otherwise it's cleared.  Not sure whether that would be of any interest though :)

::: js/src/jit/shared/Assembler-x86-shared.h
@@ +1118,5 @@
> +            MOZ_ASSUME_UNREACHABLE("unexpected operand kind");
> +        }
> +    }
> +    void lock_incl(const Operand &op) {
> +        switch (op.kind()) {

The lock() call could be hoisted above the switch.

@@ +1143,5 @@
> +            MOZ_ASSUME_UNREACHABLE("unexpected operand kind");
> +        }
> +    }
> +    void lock_decl(const Operand &op) {
> +        switch (op.kind()) {

Same here.

Comment 2

[Sean Stangl [:sstangl]]

> The only difference would be that the
> spewing will show the lock in a separate line, which is good too, as it's
> closer to what a disassembler would show.

GDB's disassembler shows "lock" as a prefix to the instruction, as it is in the bytestream, e.g., "lock incl".

I was concerned with people incorrectly using instructions without providing locking -- for example, "xadd" is entirely pointless without a lock prefix, and although "cmpxchg" can be used without locking the bus, that's probably not what is meant.

I'm OK with this change, but everyone has to promise to be careful.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: patch v2

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8371812 [details] [diff] [review]
patch v2

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

Thanks! For instructions that don't make sense without the lock prefix, it might be nice to call lock() directly in their body, but I am fine as it is now. I am curious to see what will be done with that :)

Comment 5

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb8a9d072a6

Thanks!

Comment 6

[Carsten Book [:Tomcat]]

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