AArch64 atomic operations disrupted by fast-regalloc

[TNorthover]

Bugzilla Link	25526
Resolution	FIXED
Resolved on	Nov 19, 2016 14:13
Version	trunk
OS	All
CC	@compnerd,@emaste,@jyknight,@kbeyls,@TNorthover

Extended Description

When compiling this at -O0:

define {i64, i1} @​foo(i64* %addr, i64 %desired, i64 %new) {
  %old = cmpxchg i64* %addr, i64 %desired, i64 %new seq_cst seq_cst
  ret {i64, i1} %old
}

Fast-regalloc seems to insert many spills between the ldaxr and the stlxr. If these are on the same cache-line as %addr (in practice) then they can reset the monitor causing the operation to fail.

ARM is almost certainly affected too. But both only at -O0.

I'm still not sure what to do about this one. Some obvious options are:

1. Go back to lots of pseudo-instructions for various different uses. Extremely nasty, we worked very hard to get rid of those for better codegen.
2. Disable fast-regalloc in all cases. Probably bad for debug quality, so I doubt this is viable.
3. Emit libcalls at -O0. I'm not sure all platforms support these though, we'd have to make sure they're available.

[TNorthover]

assigned to @TNorthover

[TNorthover]

*** Bug llvm/llvm-bugzilla-archive#25611 has been marked as a duplicate of this bug. ***

[TNorthover]

Current GCC does not include any atomic primitives in libgcc, so as tempting as punting to a libcall would be we'd break compatibility with GCC <= 5.3 even if we could convince them to add it. So I don't think that's viable.

Disabling fast-regalloc also doesn't seem viable. It's been carefully tuned for speed and debugability. We don't want to lose that.

I also tried mangling ldxr/stxr intrinsic nodes in the DAG and implementing support directly in FastISel. Neither solved the fundamental liveness issue.

So I've reluctantly come to think reimplementing a limited subset of the pseudo-instruction path (expanded post-regalloc) is the only real solution. Fortunately I think we can ignore some of the complexities by treating all operations as seq_cst. Also fortunately, I don't think other atomicrmw operations are affected (no live vregs across BB boundaries).

I've posted a review at http://reviews.llvm.org/D16239.

[kbeyls]

*** Bug #22647 has been marked as a duplicate of this bug. ***

[kbeyls]

The same issue also exists for AArch32. Maybe this ticket should cover both AArch64 and AArch32?

[TNorthover]

The AArch64 side is fixed now (r266339). ARM's going to be messier, unfortunately.

[TNorthover]

And the 32-bit side is fixed in r266679. Yuck!

[jyknight]

Ugh -- this seems really bad, indeed. I don't see how this can actually even be safe in normal codegen.

E.g., this test, demonstrating the codegen under extreme register pressure, results in the core loop having a spill in it between ldrexb and strexb, which I believe will have the same problem, right?

define { i8, i1 } @​test_cmpxchg_8(i8* %addr, i8 %desired, i8 %new) nounwind {
%tmp = call {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} asm sideeffect "", "={r0},={r1},={r2},={r3},={r4},={r5},={r6},={r7},={r8},={r9},={r10},={r11},={r12},={lr}"()
%res = cmpxchg i8* %addr, i8 %desired, i8 %new seq_cst monotonic
%t0 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 0
%t1 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 1
%t2 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 2
%t3 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 3
%t4 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 4
%t5 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 5
%t6 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 6
%t7 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 7
%t8 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 8
%t9 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 9
%t10 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 10
%t11 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 11
%t12 = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 12
%tlr = extractvalue {i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32} %tmp, 13
call void asm sideeffect "", "{r0},{r1},{r2},{r3},{r4},{r5},{r6},{r7},{r8},{r9},{r10},{r11},{r12},{lr}"(i32 %t0, i32 %t1, i32 %t2, i32 %t3, i32 %t4, i32 %t5, i32 %t6, i32 %t7, i32 %t8, i32 %t9, i32 %t10, i32 %t11, i32 %t12, i32 %tlr)
ret { i8, i1 } %res
}

.LBB0_2: @ %cmpxchg.trystore
@ =>This Inner Loop Header: Depth=1
strexb r3, r11, [r1]
cmp r3, #​0
beq .LBB0_6
@ BB#3: @ %cmpxchg.releasedload
@ in Loop: Header=BB0_2 Depth=1
ldrexb r3, [r1]
str r3, [sp, #​36] @ 4-byte Spill
cmp r3, r2
beq .LBB0_2
b .LBB0_5

[jyknight]

Forgot to note, that test was with "llc -mtriple=armv7-linux-gnu", that is, the default -O2 not O0.

[TNorthover]

Bugger, yes. I was really hoping such an example didn't exist.

[TNorthover]

mentioned in issue llvm/llvm-bugzilla-archive#25611