On Wed, Jan 17, 2018 at 12:36:31AM -0800, Noah Misch wrote:
> On Tue, Jan 16, 2018 at 08:50:24AM -0800, Andres Freund wrote:
> > On 2018-01-16 16:12:11 +0900, Michael Paquier wrote:
> > > On Fri, Feb 03, 2017 at 12:26:50AM +0000, Noah Misch wrote:
> > > > Since this emits double syncs with older xlc, I recommend instead replacing
> > > > the whole thing with inline asm. As I opined in the last message of the
> > > > thread you linked above, the intrinsics provide little value as abstractions
> > > > if one checks the generated code to deduce how to use them. Now that the
> > > > generated code is xlc-version-dependent, the port is better off with
> > > > compiler-independent asm like we have for ppc in s_lock.h.
> > >
> > > Could it be cleaner to just use __xlc_ver__ to avoid double syncs on
> > > past versions? I think that it would make the code more understandable
> > > than just listing directly the instructions.
> >
> > Given the quality of the intrinsics on AIX, see past commits and the
> > comment in the code quoted above, I think we're much better of doing
> > this via inline asm.
>
> For me, verifiability is the crucial benefit of inline asm. Anyone with an
> architecture manual can thoroughly review an inline asm implementation. Given
> intrinsics and __xlc_ver__ conditionals, the same level of review requires
> access to every xlc version.
> > > As Heikki is not around these days, Noah, could you provide a new
> > > version of the patch? This bug has been around for some time now, it
> > > would be nice to move on..
>
> Not soon.
Done. fetch-add-variable-test-v1.patch just adds tests for non-constant
addends and 16-bit edge cases. Today's implementation handles those,
PostgreSQL doesn't use them, and I might easily have broken them.
fetch-add-xlc-asm-v1.patch moves xlc builds from the __fetch_and_add()
intrinsic to inline asm. fetch-add-gcc-xlc-unify-v1.patch moves fetch_add to
inline asm for all other ppc compilers. gcc-7.2.0 generates equivalent code
before and after. I plan to keep the third patch HEAD-only, back-patching the
other two. I tested with xlc v12 and v13.