[X86] Allow atomic operations using immediates to avoid using a register

[morisset]

Summary

The only valid lowering of atomic stores in the X86 backend was mov from
register to memory. As a result, storing an immediate required a useless copy
of the immediate in a register. Now these can be compiled as a simple mov.

Similarily, adding/and-ing/or-ing/xor-ing an
immediate to an atomic location (but through an atomic_store/atomic_load,
not a fetch_whatever intrinsic) can now make use of an 'add $imm, x(%rip)'
instead of using a register. And the same applies to inc/dec.

This second point matches the first issue identified in

http://llvm.org/bugs/show_bug.cgi?id=17281

[reames]

I don't see an obvious errors in this change, but am not familiar with the areas you're touching in detail. I believe the memory model aspects to be correct, but can not speak for the x86 backend details.

It's worth noting that these instructions are specifically ordered *operations* not ordered *fences*. There's no need to issue a StoreLoad barrier following the release since a Load from the same address is already ordered and Loads from other locations are unordered with respect to a release *operation*.

I mention this only because I got confused reviewing the patch and it might be good to call it out in a comment. It might also be worth adding a CHECK-NOT to ensure lack of lock prefix in case someone in the future gets this wrong.

[morisset]

• Add 'CHECK-NOT: lock' (and comment) to atomic_mi.ll, based on Philip Reames suggestion
• Add three missing tests to atomic_mi.ll

[morisset]

I have visibly misunderstood how arc diff works, as this time I have lost the modifications to the backend in this update to the patch.
I will fix that as soon as possible.

[morisset]

Trying to fix my previous mistake with arcanist and make the diff reappear in Phabricator

[morisset]

Eliminating the modification to X86CodeEmitter as it was part of the JIT that just got nuked upstream

[reames]

This version LGTM pending review by someone on the backend elements.

Note: I think your refinement of the tests may have pointed out a related bug w.r.t. the missing lock prefix on sequentially consistent accesses. If so, I'm fine with addressing that separately.

test/CodeGen/X86/atomic_mi.ll

223

Shouldn't there be a lock prefix on this one to enforce the StoreLoad barrier required by cst? cst is w.r.t. all addresses (unlike every other ordering mode...)

293

Same here.

357

Again, lock prefix?

[morisset]

Actually, on X86 the xchg instruction is a bit special in that it automatically behaves as if it had a LOCK prefix, even if it is not explicit (source: Intel manual, 2B 4-71)
So I don't think it is a bug that it is not added.
Do you think I should add a comment somewhere to that effect? And if so where?

Thanks again for your reviews!

[reames]

In D4796#14, @morisset wrote:

Actually, on X86 the xchg instruction is a bit special in that it automatically behaves as if it had a LOCK prefix, even if it is not explicit (source: Intel manual, 2B 4-71)
So I don't think it is a bug that it is not added.

You're right. I had forgotten this. Sorry for the noise.

Do you think I should add a comment somewhere to that effect? And if so where?

A note in the test might be warranted. It's valid to either have the lock prefix or not in the places I pointed out before.

Thanks again for your reviews!

Glad to help where I can. I really really really don't want to be debugging these type of issues later. :)

[morisset]

Add back the modification to X86CodeEmitter as the Jit resurrected

+ an extra comment on the implicitness of the lock prefix before xchg.

[morisset]

Got a lgtm from Nadav Rotem on the mailing list.

[morisset]

Commited r216980

test/CodeGen/X86/atomic_mi.ll

 ; The binary operations supported are currently add, and, or, xor.
 ; sub is not supported because they are translated by an addition of the
 ; negated immediate.
 ; Finally, we also check the same kind of pattern for inc/dec
 
 ; seq_cst stores are left as (lock) xchgl, but we try to check every other
 ; attribute at least once.
 
+; Please note that these operations do not require the lock prefix: only
+; sequentially consistent stores require this kind of protection on X86.
+
 define void @store_atomic_imm_8(i8* %p) {
 ; X64-LABEL: store_atomic_imm_8
 ; X64: movb
 ; X64-NOT: movb
 ; X32-LABEL: store_atomic_imm_8
 ; X32: movb
 ; X32-NOT: movb
   store atomic i8 42, i8* %p release, align 1
; X32: xchgl
   store atomic i32 42, i32* %p seq_cst, align 4
   ret void
 }
 
 ; ----- ADD -----
 
 define void @add_8(i8* %p) {
 ; X64-LABEL: add_8
+; X64-NOT: lock
 ; X64: addb
 ; X64-NOT: movb
 ; X32-LABEL: add_8
+; X32-NOT: lock
 ; X32: addb
 ; X32-NOT: movb
   %1 = load atomic i8* %p seq_cst, align 1
   %2 = add i8 %1, 2
   store atomic i8 %2, i8* %p release, align 1
   ret void
 }
 
 define void @add_16(i16* %p) {
 ;   Currently the transformation is not done on 16 bit accesses, as the backend
 ;   treat 16 bit arithmetic as expensive on X86/X86_64.
 ; X64-LABEL: add_16
 ; X64-NOT: addw
 ; X32-LABEL: add_16
 ; X32-NOT: addw
   %1 = load atomic i16* %p acquire, align 2
   %2 = add i16 %1, 2
   store atomic i16 %2, i16* %p release, align 2
   ret void
 }
 
 define void @add_32(i32* %p) {
 ; X64-LABEL: add_32
+; X64-NOT: lock
 ; X64: addl
 ; X64-NOT: movl
 ; X32-LABEL: add_32
+; X32-NOT: lock
 ; X32: addl
 ; X32-NOT: movl
   %1 = load atomic i32* %p acquire, align 4
   %2 = add i32 %1, 2
   store atomic i32 %2, i32* %p monotonic, align 4
   ret void
 }
 
 define void @add_64(i64* %p) {
 ; X64-LABEL: add_64
+; X64-NOT: lock
 ; X64: addq
 ; X64-NOT: movq
 ;   We do not check X86-32 as it cannot do 'addq'.
 ; X32-LABEL: add_64
   %1 = load atomic i64* %p acquire, align 8
   %2 = add i64 %1, 2
   store atomic i64 %2, i64* %p release, align 8
   ret void
; X32: xchgl
   store atomic i32 %2, i32* %p seq_cst, align 4
   ret void
 }
 
 ; ----- AND -----
 
 define void @and_8(i8* %p) {
 ; X64-LABEL: and_8
+; X64-NOT: lock
 ; X64: andb
 ; X64-NOT: movb
 ; X32-LABEL: and_8
+; X32-NOT: lock
 ; X32: andb
 ; X32-NOT: movb
   %1 = load atomic i8* %p monotonic, align 1
   %2 = and i8 %1, 2
   store atomic i8 %2, i8* %p release, align 1
   ret void
 }
 
 define void @and_16(i16* %p) {
 ;   Currently the transformation is not done on 16 bit accesses, as the backend
 ;   treat 16 bit arithmetic as expensive on X86/X86_64.
 ; X64-LABEL: and_16
 ; X64-NOT: andw
 ; X32-LABEL: and_16
 ; X32-NOT: andw
   %1 = load atomic i16* %p acquire, align 2
   %2 = and i16 %1, 2
   store atomic i16 %2, i16* %p release, align 2
   ret void
 }
 
 define void @and_32(i32* %p) {
 ; X64-LABEL: and_32
+; X64-NOT: lock
 ; X64: andl
 ; X64-NOT: movl
 ; X32-LABEL: and_32
+; X32-NOT: lock
 ; X32: andl
 ; X32-NOT: movl
   %1 = load atomic i32* %p acquire, align 4
   %2 = and i32 %1, 2
   store atomic i32 %2, i32* %p release, align 4
   ret void
 }
 
 define void @and_64(i64* %p) {
 ; X64-LABEL: and_64
+; X64-NOT: lock
 ; X64: andq
 ; X64-NOT: movq
 ;   We do not check X86-32 as it cannot do 'andq'.
 ; X32-LABEL: and_64
   %1 = load atomic i64* %p acquire, align 8
   %2 = and i64 %1, 2
   store atomic i64 %2, i64* %p release, align 8
   ret void
 }
 
+define void @and_32_seq_cst(i32* %p) {
+; X64-LABEL: and_32_seq_cst
+; X64: xchgl
+; X32-LABEL: and_32_seq_cst
+; X32: xchgl
+  %1 = load atomic i32* %p monotonic, align 4
+  %2 = and i32 %1, 2
+  store atomic i32 %2, i32* %p seq_cst, align 4
+  ret void
+}
+
 ; ----- OR -----
 
 define void @or_8(i8* %p) {
 ; X64-LABEL: or_8
+; X64-NOT: lock
 ; X64: orb
 ; X64-NOT: movb
 ; X32-LABEL: or_8
+; X32-NOT: lock
 ; X32: orb
 ; X32-NOT: movb
   %1 = load atomic i8* %p acquire, align 1
   %2 = or i8 %1, 2
   store atomic i8 %2, i8* %p release, align 1
   ret void
 }
 
 define void @or_16(i16* %p) {
 ; X64-LABEL: or_16
 ; X64-NOT: orw
 ; X32-LABEL: or_16
 ; X32-NOT: orw
   %1 = load atomic i16* %p acquire, align 2
   %2 = or i16 %1, 2
   store atomic i16 %2, i16* %p release, align 2
   ret void
 }
 
 define void @or_32(i32* %p) {
 ; X64-LABEL: or_32
+; X64-NOT: lock
 ; X64: orl
 ; X64-NOT: movl
 ; X32-LABEL: or_32
+; X32-NOT: lock
 ; X32: orl
 ; X32-NOT: movl
   %1 = load atomic i32* %p acquire, align 4
   %2 = or i32 %1, 2
   store atomic i32 %2, i32* %p release, align 4
   ret void
 }
 
 define void @or_64(i64* %p) {
 ; X64-LABEL: or_64
+; X64-NOT: lock
 ; X64: orq
 ; X64-NOT: movq
 ;   We do not check X86-32 as it cannot do 'orq'.
 ; X32-LABEL: or_64
   %1 = load atomic i64* %p acquire, align 8
   %2 = or i64 %1, 2
   store atomic i64 %2, i64* %p release, align 8
   ret void
 }
 
+define void @or_32_seq_cst(i32* %p) {
+; X64-LABEL: or_32_seq_cst
+; X64: xchgl
+; X32-LABEL: or_32_seq_cst
+; X32: xchgl
+  %1 = load atomic i32* %p monotonic, align 4
+  %2 = or i32 %1, 2
+  store atomic i32 %2, i32* %p seq_cst, align 4
+  ret void
+}
+
 ; ----- XOR -----
 
 define void @xor_8(i8* %p) {
 ; X64-LABEL: xor_8
+; X64-NOT: lock
 ; X64: xorb
 ; X64-NOT: movb
 ; X32-LABEL: xor_8
+; X32-NOT: lock
 ; X32: xorb
 ; X32-NOT: movb
   %1 = load atomic i8* %p acquire, align 1
   %2 = xor i8 %1, 2
   store atomic i8 %2, i8* %p release, align 1
   ret void
 }
 
 define void @xor_16(i16* %p) {
 ; X64-LABEL: xor_16
 ; X64-NOT: xorw
 ; X32-LABEL: xor_16
 ; X32-NOT: xorw
   %1 = load atomic i16* %p acquire, align 2
   %2 = xor i16 %1, 2
   store atomic i16 %2, i16* %p release, align 2
   ret void
 }
 
 define void @xor_32(i32* %p) {
 ; X64-LABEL: xor_32
+; X64-NOT: lock
 ; X64: xorl
 ; X64-NOT: movl
 ; X32-LABEL: xor_32
+; X32-NOT: lock
 ; X32: xorl
 ; X32-NOT: movl
   %1 = load atomic i32* %p acquire, align 4
   %2 = xor i32 %1, 2
   store atomic i32 %2, i32* %p release, align 4
   ret void
 }
 
 define void @xor_64(i64* %p) {
 ; X64-LABEL: xor_64
+; X64-NOT: lock
 ; X64: xorq
 ; X64-NOT: movq
 ;   We do not check X86-32 as it cannot do 'xorq'.
 ; X32-LABEL: xor_64
   %1 = load atomic i64* %p acquire, align 8
   %2 = xor i64 %1, 2
   store atomic i64 %2, i64* %p release, align 8
   ret void
 }
 
+define void @xor_32_seq_cst(i32* %p) {
+; X64-LABEL: xor_32_seq_cst
+; X64: xchgl
+; X32-LABEL: xor_32_seq_cst
+; X32: xchgl
+  %1 = load atomic i32* %p monotonic, align 4
+  %2 = xor i32 %1, 2
+  store atomic i32 %2, i32* %p seq_cst, align 4
+  ret void
+}
+
 ; ----- INC -----
 
 define void @inc_8(i8* %p) {
 ; X64-LABEL: inc_8
+; X64-NOT: lock
 ; X64: incb
 ; X64-NOT: movb
 ; X32-LABEL: inc_8
+; X32-NOT: lock
 ; X32: incb
 ; X32-NOT: movb
   %1 = load atomic i8* %p seq_cst, align 1
   %2 = add i8 %1, 1
   store atomic i8 %2, i8* %p release, align 1
   ret void
 }
 
 define void @inc_16(i16* %p) {
 ;   Currently the transformation is not done on 16 bit accesses, as the backend
 ;   treat 16 bit arithmetic as expensive on X86/X86_64.
 ; X64-LABEL: inc_16
 ; X64-NOT: incw
 ; X32-LABEL: inc_16
 ; X32-NOT: incw
   %1 = load atomic i16* %p acquire, align 2
   %2 = add i16 %1, 1
   store atomic i16 %2, i16* %p release, align 2
   ret void
 }
 
 define void @inc_32(i32* %p) {
 ; X64-LABEL: inc_32
+; X64-NOT: lock
 ; X64: incl
 ; X64-NOT: movl
 ; X32-LABEL: inc_32
+; X32-NOT: lock
 ; X32: incl
 ; X32-NOT: movl
   %1 = load atomic i32* %p acquire, align 4
   %2 = add i32 %1, 1
   store atomic i32 %2, i32* %p monotonic, align 4
   ret void
 }
 
 define void @inc_64(i64* %p) {
 ; X64-LABEL: inc_64
+; X64-NOT: lock
 ; X64: incq
 ; X64-NOT: movq
 ;   We do not check X86-32 as it cannot do 'incq'.
 ; X32-LABEL: inc_64
   %1 = load atomic i64* %p acquire, align 8
   %2 = add i64 %1, 1
   store atomic i64 %2, i64* %p release, align 8
   ret void
; X32: xchgl
   store atomic i32 %2, i32* %p seq_cst, align 4
   ret void
 }
 
 ; ----- DEC -----
 
 define void @dec_8(i8* %p) {
 ; X64-LABEL: dec_8
+; X64-NOT: lock
 ; X64: decb
 ; X64-NOT: movb
 ; X32-LABEL: dec_8
+; X32-NOT: lock
 ; X32: decb
 ; X32-NOT: movb
   %1 = load atomic i8* %p seq_cst, align 1
   %2 = sub i8 %1, 1
   store atomic i8 %2, i8* %p release, align 1
   ret void
 }
 
 define void @dec_16(i16* %p) {
 ;   Currently the transformation is not done on 16 bit accesses, as the backend
 ;   treat 16 bit arithmetic as expensive on X86/X86_64.
 ; X64-LABEL: dec_16
 ; X64-NOT: decw
 ; X32-LABEL: dec_16
 ; X32-NOT: decw
   %1 = load atomic i16* %p acquire, align 2
   %2 = sub i16 %1, 1
   store atomic i16 %2, i16* %p release, align 2
   ret void
 }
 
 define void @dec_32(i32* %p) {
 ; X64-LABEL: dec_32
+; X64-NOT: lock
 ; X64: decl
 ; X64-NOT: movl
 ; X32-LABEL: dec_32
+; X32-NOT: lock
 ; X32: decl
 ; X32-NOT: movl
   %1 = load atomic i32* %p acquire, align 4
   %2 = sub i32 %1, 1
   store atomic i32 %2, i32* %p monotonic, align 4
   ret void
 }
 
 define void @dec_64(i64* %p) {
 ; X64-LABEL: dec_64
+; X64-NOT: lock
 ; X64: decq
 ; X64-NOT: movq
 ;   We do not check X86-32 as it cannot do 'decq'.
 ; X32-LABEL: dec_64
   %1 = load atomic i64* %p acquire, align 8
   %2 = sub i64 %1, 1
   store atomic i64 %2, i64* %p release, align 8
   ret void