On Mon, Nov 20, 2017 at 07:22:15PM +0000, Steve Ellcey wrote: > On Mon, 2017-11-20 at 18:27 +0000, James Greenhalgh wrote: > > > > If you have the time, would you mind giving me a quick run-down of what > > design decisions went in to this patch, and why they are the right thing > > to do? Sorry to offload that, but it will be the most efficient route > > to a review. > > The main design decision was to use the existing IFUNC infrastructure > that is used on ARM32 to enable atomic instructions that were added > with armv7-a, on i386 to enable instructions added with i586, and on > x86_64 to enable instructions added with cx16. > > The basic idea for all these is to allow users who create programs that > use the atomic_* functions to use new instructions on machines that > support them while also working on older machines that do not support > them and to not have to create two separate executables. > > Some atomic_* functions get inlined into programs, and those will > either use or not use LSE instructions based on the compiler arguments > used during compilations. If you want your program to work on all > machines you have to not compile for LSE intructions. But other > functions (or all functions if -fno-inline-atomics is used) will call > the libatomic library. Currently those functions do not use LSE > instructions but with this patch we can use the IFUNC infrastructure to > check for LSE support and use LSE in libatomic on machines where it is > supported or not use it on machines where it is not supported. > > As an example of what this change does, __atomic_compare_exchange_8 will > turn into a call to libat_compare_exchange_8_i1 on a machine that supports > LSE: > > 0000000000000000 <libat_compare_exchange_8_i1>: > 0: f9400023 ldr x3, [x1] > 4: aa0303e4 mov x4, x3 > 8: c8e4fc02 casal x4, x2, [x0] > c: eb03009f cmp x4, x3 > 10: 1a9f17e0 cset w0, eq > 14: 35000040 cbnz w0, 1c <libat_compare_exchange_8_i1+0x1c> > 18: f9000024 str x4, [x1] > 1c: d65f03c0 ret > > But call libat_compare_exchange_8 on a machine without LSE: > > 0000000000000000 <libat_compare_exchange_8>: > 0: f9400023 ldr x3, [x1] > 4: c85ffc04 ldaxr x4, [x0] > 8: eb03009f cmp x4, x3 > c: 54000061 b.ne 18 <libat_compare_exchange_8+0x18> > 10: c805fc02 stlxr w5, x2, [x0] > 14: 35ffff85 cbnz w5, 4 <libat_compare_exchange_8+0x4> > 18: 1a9f17e0 cset w0, eq > 1c: 34000040 cbz w0, 24 <libat_compare_exchange_8+0x24> > 20: d65f03c0 ret > 24: f9000024 str x4, [x1] > 28: d65f03c0 ret > 2c: d503201f nop
Thanks for the detailed explanation. I understood this, and my opinion is that the AArch64 parts of this patch are OK (and I don't know who needs to Ack the small generic changes you require). Let's give Richard/Marcus 48 hours to object while we wait for an OK on the generic bits, and then OK for AArch64. Thanks, James Reviewed-By: James Greenhalgh <james.greenha...@arm.com>
