[PATCH] D47553: Add TargetLowering::shouldExpandAtomicToLibCall and query it from AtomicExpandPass

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

In https://reviews.llvm.org/D47553#1118083, @asb wrote:

> In https://reviews.llvm.org/D47553#1118067, @efriedma wrote:
>
> > But even if they are lock-free, it still requires linking to the shared library libatomic; we don't want to impose that requirement on users if it isn't necessary.
>
>
> Excellent point. That's definitely enough motivation to avoid calling the `__atomic_*` libcalls when not strictly necessary. I'll rework my patches to lower to ll/sc for small atomics on RV32IA from the start. Of course we then run into the problem encountered by Mips - the potential for a stack spill to be inserted between the ll/sc and to break things.


If you haven't seen it, you might want to read the thread I started a while ago, http://lists.llvm.org/pipermail/llvm-dev/2016-May/099490.html discussing how AtomicExpandPass's feature of generating LL/SC loops at the IR level is a bad idea, and we need a better mechanism in order for it to generate actually-correct code. (We really ought to not expose ll and sc primitives to the IR level at all, because it's impossible to use them correctly).

That fundamental issue hasn't actually been resolved yet -- the current behavior seems to "work well enough" usually. But, I don't know whether the RISCV hardware requires strict adherence to its spec of max 16 base-ISA instructions, except loads, stores, or taken-branches. If so, the generic code in AtomicExpandPass cannot make that guarantee.


https://reviews.llvm.org/D47553