[PATCH] D45321: [atomics] Fix runtime calls for misaligned atomics

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: compiler-rt/lib/builtins/atomic.c:158
         /* FIXME: __uint128_t isn't available on 32 bit platforms.
         LOCK_FREE_ACTION(__uint128_t);*/\
       }\
----------------
efriedma wrote:
> t.p.northover wrote:
> > efriedma wrote:
> > > We also need to fix this FIXME, for correctness... if we have a 16-byte atomic store implementation, we need to use it.
> > Any chance I could skip that for now?
> > 
> > It's an order of magnitude harder than fixing the misalignment problems since LLVM already generates a mixture of libcalls and cmpxchg based on the CPU for x86, which forces this to be a runtime CPUID check.
> > 
> > 
> > 
> > 
> Can you at least fix it for targets where 16-byte atomics are always lock-free, like aarch64?  (Of course I don't expect you to implement the x86 cpuid bits.)
Sounds like a reasonable compromise. This check is easy enough to get right (I can use `defined(__SIZEOF_INT128__)` as a proxy for whether it's supported).

I don't suppose you have any ideas about the `IS_LOCK_FREE_16` check though? I had a bit of a think earlier on and it's harder than it looks. The best I've come up with so far is:

    #define IS_LOCK_FREE_16(ptr) (__builtin_constant_p(__c11_atomic_is_lock_free(16)) && __c11_atomic_is_lock_free(16) && (uintptr_t)ptr % 16 == 0)

The constant check is there because otherwise we get a realised (and unresolved) call to `__atomic_is_lock_free(16, 0)` when Clang doesn't know -- and on x86 this would, of course, be CPUID based if implemented so I can't in good conscience make Clang decide it's 0.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D45321