[PATCH] D18201: Switch over targets to use AtomicExpandPass, and clean up target atomics code.

[James Y Knight via llvm-commits]

jyknight added a comment.

In http://reviews.llvm.org/D18201#376627, @t.p.northover wrote:

> > Targets call initSyncLibcalls() to request them where they're supported and required. (This is ARM and MIPS16 at the moment)
>
>
> What's the benefit here, performance? I can't seem to find where they'd ever be used unset (only getATOMIC/getSYNC references them, and all callees I can find assert that they get a valid libcall).


I added this because I found that they were getting emitted in error on targets that should not be using them (in intermediate versions of this patch). So, I added that as essentially a safety check, to make it impossible for those expansions to be used where they should not be, which is almost everywhere.

> > Removed unnecessary selection dag expansions of ATOMIC_LOAD into ATOMIC_CMP_SWAP and ATOMIC_STORE into ATOMIC_SWAP. Targets that don't support atomic load/store should now be handled by the translation in AtomicExpandPass into atomic_load/atomic_store, rather than expanding into unnatural operations.

> 

> 

> What if the target doesn't have those libcalls? Darwin doesn't, for example, and we're going to have to allow deploying back to older OSs for quite a while even if we added them immediately (and, as you say, they have to be in a .dylib so we can't ship them with the compiler).


To the more general question "what happens if a user uses an atomic function which requires an __atomic libcall on a platform that doesn't provide it": they'll get a linker error about it not being defined.

But that doesn't have anything to do with the ATOMIC_LOAD/ATOMIC_STORE dag expansions. The __atomic_* libcalls are only generated by AtomicExpandPass for atomics that are too large, or misaligned.

So the DAG expansions would only come into play if you somehow had a platform which claims (via setMaxAtomicSizeSupported) backend support for that size, but doesn't even have an atomic load or store instruction capable of that size. I know of no platform where that'd be possibly useful. Certainly none that Darwin supports.

That's not to say that these expansions didn't USED to get used -- they did, prior to this change. For two reasons:

1stly, most obviously, that AtomicExpandPass didn't take care of the "no atomics at all of this size" case before getting to ISel. That issue is now gone.

2ndly (and probably where your concern is coming from): On ARM, LLVM used to go through that expansion for some CPU models. That was never actually necessary, though, and this patch changes it to use a normal load/store instruction, with an appropriate barrier, instead. (And if a barrier instruction isn't available, it'll use a barrier libcall.)

E.g. see the changed behavior in test/CodeGen/ARM/atomic-load-store.ll:

   %val = load atomic i32, i32* %ptr seq_cst, align 4
  -; THUMBONE: __sync_val_compare_and_swap_4
  +; THUMBONE: ldr
  +; THUMBONE: __sync_synchronize

And test/CodeGen/ARM/atomic-op.ll:

   store atomic i32 %val1, i32* %mem1 release, align 4
   store atomic i32 %val2, i32* %mem2 release, align 4
  -; CHECK-M0: ___sync_lock_test_and_set
  -; CHECK-M0: ___sync_lock_test_and_set
  +; CHECK-M0: dmb
  +; CHECK-M0: str r1, [r0]
  +; CHECK-M0: dmb
  +; CHECK-M0: str r3, [r2]

So, in summary, I don't think Darwin has anything to worry about here.


http://reviews.llvm.org/D18201