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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In https://reviews.llvm.org/D47553#1119021, @jyknight wrote:

> 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.


Thanks James, yes I recently re-read that post and had a good look at what ARM, AArch64 and Mips are doing here. Mips will move to lowering in a post-RA pseudo-expansion pas with https://reviews.llvm.org/D31287, similar to the ARM/AArch64 -O0 approach.

My current implementation is not using the shouldExpandAtomic*InIR hooks to avoid the problems you outline. I'm going the post-RA pseudo-lowering route currently. That still leaves a concern about MachineBlockPlacement, but this shouldn't be a realistic concern if you set static branch probabilities appropriately. It would be nice to not have this inter-dependency of course, but it's probably not the greatest evil. I haven't properly examined the potential for the MachineOutliner to break load-linked/store-conditional pairs.

It's somewhat tempting to either emit inline assembly or to expand the pseudo MachineInstruction only when lowering to MCInst.


https://reviews.llvm.org/D47553