[Bug target/65697] __atomic memory barriers not strong enough for __sync builtins

["amacleod at redhat dot com" <gcc-bugzilla at gcc dot gnu dot org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697

--- Comment #31 from Andrew Macleod <amacleod at redhat dot com> ---
(In reply to mwahab from comment #30)
> (In reply to James Greenhalgh from comment #28)
> 
> > I don't think this is particularly onerous for a target. The tough part in
> > all of this is figuring out the minimal cost instructions at the ISA level
> > to use for the various __atomic primitives. Extending support to a stronger
> > model, should be straightforward given explicit documentation of the
> > stronger ordering requirements.
> >
> 
> My objection to using sync patterns is that it does take more work, both for
> the initial implementation and for continuing maintenance. It also means
> adding sync patterns to targets that would not otherwise need them and
> preserving a part of the gcc infrastructure that is only needed for a legacy
> feature and could otherwise be targeted for removal.
> 

Targets that don't need special sync patterns (ie most of them) simply don't
provide them.   The expanders see no sync pattern and use SEQ_CST, exactly like
they do today.

sync patterns would only be provided by targets which do need to do something
different.   This means there is no potential bug introduction on unaffected
targets.. 

> > This also gives us an easier route to fixing any issues with the
> > acquire/release __sync primitives (__sync_lock_test_and_set and
> > __sync_lock_release) if we decide that these also need to be stronger than
> > their C++11 equivalents.
> 
> This seems like a chunky workaround to avoid having to add a barrier
> representation to gcc.  It's a, not necessarily straightforward, reworking
> of the middle-end to use patterns that would need to be added to
> architectures that don't currently have them and at least checked in the
> architectures that do.

Well, it actually returns back to the situation before they were merged. We use
to look for sync patterns too... until I thought they were redundant.

> 
> This seems to come down to what enum memmodel is supposed to be for. If it
> is intended to represent the barriers provided by C11/C+11 atomics and
> nothing else than a workaround seems unavoidable. If it is meant to
> represent barriers needed by gcc to compile user code than, it seems to me,
> that it would be better to just add the barrier to the enum and update code
> where necessary. 
> 
> Since memmodel is relatively recent, was merged from what looks like the C++
> memory model branch (cxx-mem-model), and doesn't seem to have changed since
> then, it's maybe not surprising that it doesn't already include every thing
> needed by gcc. I don't see that adding to it should be prohibited, provided
> the additions can be show to be strictly required.

The intention was to deprecate __sync and support just the c++ memory model. 
SEQ_CST == SYNC was the original intent and understanding, thus the code merge.
 If we decide we want/need to provide a stronger form of SEQ_CST and call it
SYNC, then we can do that as required... but I'm not envisioning a lot of
future additional memory models. Although never say never I suppose.