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

[amacleod at redhat dot com]

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

Andrew Macleod <amacleod at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #35425|0                           |1
        is obsolete|                            |

--- Comment #50 from Andrew Macleod <amacleod at redhat dot com> ---
Created attachment 35478
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35478&action=edit
implement SYNC flag for memory model

> I've been working on patches for this. Adding a new MEMMODEL type was my
> first approach because it means that gcc has one representation for all the
> memory models it supports. I decided not to submit the patch because Aarch64
> also needs an tag for the __sync acquire used in __sync_lock_test_and_set.
> Adding that touches more backends than the MEMMODEL_SYNC and some of those
> changes are not obvious (for the mips backend in particular). The reasons
> why Aarch64 needs a new acquire barrier are rather tenuous (they're in the
> earlier comments) and don't seem to justify the possibly invasive changes
> that would be needed. 
> 
> The approach I'm working on now is to add a target hook to allow a backend
> to supply its own expansion of calls to the __sync builtins. That makes for
> smaller changes in the target-independent code and lets the Aarch64 backend
> add the new barriers as target-specific memory models. The actual code
> generation goes through the infrastructure for the atomics.
> 
> Adding the __sync barriers to coretypes.h is the better approach if more
> than one backend has this problem. Since it seems that only Aarch64 is
> affected, I think its better to go with the target hook. If a new
> architecture gets added with the same problem, it will be easy enough to
> switch to the coretypes.h approach.

Well, if it affects some target now, it likely to affect some target in the
future as well. Didn't someone also mention there is a potential issue with PPC
somewhere too?

In any case, I'm not a fan of adding target hooks for this... We ought to just
bite the bullet and integrate it cleanly into the memory model. 

I have a patch which adds a SYNC flag to the upper bit of the memory model. It
modifies all the generic code and target patterns to use access routines
(declared in tree.h) instead of hard coding masks and flags (which probably
should have be done in the first place). This makes carrying around the SYNC
flag transparent until you want to look for it.

There are new sync enum variants for SYNC_ACQUIRE, SYNC_SEQ_CST, and
SYNC_RELEASE. 

The new access routines ignore the sync flag, so "is_mm_acquire (model)" will
be true for MEMMODEL_ACQUIRE and MEMMODEL_SYNC_ACQUIRE. If a target doesn't
care about differentiating sync (which is most targets) it will be transparent.

It is also simple to check for the presence of sync "is_mm_sync (model)", or a
particular MEMMODEL_SYNC variant "model == MEMMODEL_SYNC_SEQ_CST". A quick hack
to a couple of x86 patterns indicate this works and I can issue extra/different
barriers for sync variants.

This compiles on all targets, but is only runtime tested on
x86_64-unknown-linux-gnu with no regressions.

With this you should be able to easily issue whatever different insn sequence
you need to within an atomic pattern.   Give it a try.  If it works as
required, then we'll see about executing on all targets for testing...